-
Notifications
You must be signed in to change notification settings - Fork 3
Reduce unnecessary replotting on dashboard startup and on target variable switching #260
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
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.
this project is steadily becoming actual software instead of a bunch of bodges in a trenchcoat and i love it.
great work! can merge as-is, questions minor
app/server.R
Outdated
@@ -131,14 +131,14 @@ server <- function(input, output, session) { | |||
HOSP_CURRENT <- resolveCurrentHospDay() | |||
|
|||
PREV_AS_OF_DATA <- reactiveVal(NULL) | |||
AS_OF_CHOICES <- reactiveVal(NULL) | |||
AS_OF_CHOICES <- reactiveVal(resolveCurrentHospDay()) |
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.
question: do we need to run resolveCurrentHospDay
again, or can we use the recently-computed value from HOSP_CURRENT
?
is the advantage of computing this as a reactive value that if a user leaves the dash open for long periods of time, they'll automatically see updates?
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.
It looks to me that the main goal of AS_OF_CHOICES
being reactive is to be easy to update from child processes. It doesn't look like its reactive behavior will ever be triggered since as of choices will only change when the target variable changes, which causes the same replotting and recalculating that AS_OF_CHOICES
would.
I think we're safe to make it into a global instead, but then we'll need to do out-of-environment assignment (<<-
). Knowing that, do you have a preference one way or the other?
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.
hmmm I don't feel strongly about it -- a global vs some superfluous reactivity are probably equally annoying. that suggests, do whatever's easy :)
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.
Given that we should do a bigger refactor of the reactive behavior, I'm leaving this as-is and will create an issue.
The UI defines startup selection values globally, for all users assigned to that Shiny session. Startup values are treated as constants, so a hard-coded asof date won't update if the Shiny session persists (is continually used) across a date boundary that would normally change the default asof selection. Thus, we need to set the starting asof value on a user-by-user basis so that it is recalculated when a new dashboard instance is opened. To do that, set the initial asof value back to empty ("") and run an updater observe on dashboard startup only. In reality, this is practically unimportant; we don't have enough dashboard users to persist Shiny sessions very long.
ifelse
coercing dates into numerics; usedplyr::if_else
instead.