Feat: Graceful Handling of First-touch API Call (App List)#12
Feat: Graceful Handling of First-touch API Call (App List)#12DeepanshKhurana merged 10 commits intomainfrom
Conversation
…rame feat: create new empty_state_utils.R with function for generating new empty states
chore: update README.md with FAQs
Gotfrid
left a comment
There was a problem hiding this comment.
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 |
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.
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.
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.
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'snoDatafor the app table)The GIF below illustrates the changes/cases.
Definition of Done
R CMD check, linter, unit tests, spelling)..Rdfiles withroxygen2::roxygenise())