-
Notifications
You must be signed in to change notification settings - Fork 0
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
Bug: Disallow user to check NCA / Outputs tabs without mapping data #152
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good to me, thank you! I have added two nitpicks/suggestions to consider, but the change is fine as is.
fluid = TRUE, | ||
tab_visuals_ui("visuals") | ||
), | ||
# New TLG tab | ||
nav_panel("TLG", | ||
nav_panel("TLG", value = "tlgs", |
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: Since every other element (title, module name, module id) refers to this tab as tlg
and not tlgs
, I would keep that consistent and give it the same value.
shinyjs::enable(selector = "#page li a[data-value=nca]") | ||
shinyjs::enable(selector = "#page li a[data-value=visualisation]") | ||
shinyjs::enable(selector = "#page li a[data-value=tlgs]") |
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: In repetitive calls like this you can utilize some functional programming with purrr
:
shinyjs::enable(selector = "#page li a[data-value=nca]") | |
shinyjs::enable(selector = "#page li a[data-value=visualisation]") | |
shinyjs::enable(selector = "#page li a[data-value=tlgs]") | |
purrr::walk(c("nca", "visualisation", "tlgs"), \(tab) { | |
shinyjs::disable(selector = paste0("#page li a[data-value=", tab, "]")) | |
}) |
but in this case, considering not much code repetition, it is absolutely fine.
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.
Hey good job! I think this solution is clean and simple 👌🏽
In terms of feature I will also add a nitpick request! Could you also include a message in Review Data
that until the dataframe is uploaded says something like: "Please, remember to submit your variables mapping in Mapping and Filters
"
Issue
Closes #143
Description
When user has not mapped the data (define column names and accepted) it should not be able to go through other tabs (NCA or Outputs) as it can be confusing for them to see errror messages afterwards. Instead, those tabs should remain disable and perhpaps even show a notification
Definition of Done