-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Graceful Handling of First-touch API Call (App List) #12
Conversation
…rame feat: create new empty_state_utils.R with function for generating new empty states
chore: update README.md with FAQs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thank you for your rapid reaction to an opened issue 💯
Approved with a few non-critical comments.
Another comment (out of scope of this PR) I have is related to a pattern that I see in main.R and which I find weird:
observeEvent(..., { renderUI(mod$ui()); mod$server() }
Would it be cleaner if you create ui+server for a particular app section only once, and inside a corresponding server you decide what to render by passing a reactive key, e.g. state$selected_app()$guid
?
div, | ||
img, | ||
p, | ||
renderUI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: prefer trailing comma
app/logic/empty_state_utils.R
Outdated
text = "Select an application and a job to view logs", | ||
image_path = "static/illustrations/empty_state.svg" | ||
) { | ||
renderUI({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: i think it's better to make a function that is not shiny-specific, i.e. remove renderUI
from it. This way you can control what to do with the output of this function - use it in the server, or in the ui function.
app/main.R
Outdated
} | ||
}, ignoreInit = TRUE, ignoreNULL = TRUE) | ||
}, ignoreInit = FALSE, ignoreNULL = FALSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: ignoreInit = FALSE
is the default value, no need to specify it.
app/main.R
Outdated
alt = "Select an application and a job to view logs" | ||
) | ||
|
||
if (class(app_list()) != "data.frame") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: prefer `inherits(app_list(), "data.frame")
example:
x <- dplyr::as_tibble(mtcars)
class(x) # "tbl_df" "tbl" "data.frame"
class(x) != "data.frame" TRUE TRUE FALSE
inherits(x, "data.frame") # TRUE
Thank you @Gotfrid for all the suggestions. I've made changes and merged them. For the comment about the reactivity flow, I can't think of the original reason for taking that route in implementation. I agree with you that it is a bit awkward and can be refactored. Documented this as an enhancement issue (#13) |
Have you read the Contributing Guidelines? ✅
Closes #11
Related to #10
Description
reactableLang
'snoData
for the app table)The GIF below illustrates the changes/cases.
Definition of Done
R CMD check
, linter, unit tests, spelling)..Rd
files withroxygen2::roxygenise()
)