Skip to content
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

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

DeepanshKhurana
Copy link
Member

@DeepanshKhurana DeepanshKhurana commented Jun 11, 2024

Have you read the Contributing Guidelines?

Closes #11
Related to #10

Description

  • Handle the case where the API works but has no apps (reactableLang's noData for the app table)
  • Handle the case where the API itself fails by adding a new, more relevant empty state
  • Minor fixes for CSS and strings
  • Update README.md with FAQs

The GIF below illustrates the changes/cases.

Screen Recording 2024-06-11 at 4 04 47 PM

Definition of Done

  • The change is thoroughly documented.
  • The CI passes (R CMD check, linter, unit tests, spelling).
  • Any generated files have been updated (e.g. .Rd files with roxygen2::roxygenise())

Copy link
Member

@Gotfrid Gotfrid left a 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: prefer trailing comma

text = "Select an application and a job to view logs",
image_path = "static/illustrations/empty_state.svg"
) {
renderUI({
Copy link
Member

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)
Copy link
Member

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") {
Copy link
Member

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

@DeepanshKhurana DeepanshKhurana removed the request for review from jakubnowicki June 12, 2024 12:14
@DeepanshKhurana DeepanshKhurana merged commit e7dd624 into main Jun 12, 2024
1 check passed
@DeepanshKhurana DeepanshKhurana deleted the feat/gracefully-handle-app-list-error branch June 12, 2024 12:16
@DeepanshKhurana
Copy link
Member Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Handle First-touch API Failure Gracefully
2 participants