Shiny reactive #35
Shiny reactive #35
Conversation
|
The new tab is there, great work. I would make some changes:
Here's what I see so far:
|
|
@Robinlovelace can you please check that out now. I actually only had to made the new plot reactive; all the rest could be left as is, which is great. I'd prefer if this could be merged subject to your approval at this stage, with a second iteration working out what we can sensibly add to the second ("Health Impacts") panel. At present it's just the bare essence as proof of principle. |
R/health-calculations.R
Outdated
| download.file (u, destfile = file.path (path, nm)) | ||
| f <- system.file (nm, package = "upthat") | ||
| } | ||
| mode_shift <- read.csv (f) |
There was a problem hiding this comment.
@Robinlovelace Note that this function auto-downloads the scenario results into /inst the first time it's run. I think this is a sensible approach, as it takes no time to download, and ensures we only need to keep the one master file updated. Feel free to suggest alternative approaches if you'd prefer
There was a problem hiding this comment.
Sounds sensible, extensible and future proof, good thinking 🚀
|
@Robinlovelace ha, i didn't realise you'd started the PR when i tried but failed to request your review of this... Anyway, please review! |
R/health-calculations.R
Outdated
| download.file (u, destfile = file.path (path, nm)) | ||
| f <- system.file (nm, package = "upthat") | ||
| } | ||
| mode_shift <- read.csv (f) |
There was a problem hiding this comment.
Sounds sensible, extensible and future proof, good thinking 🚀
|
Happy to merge pending a test I plan to do now. If it's not too much effort can you make the style consistent with the rest of the package so we don't have 2 styles in 1 package? Can fix that post merge also. |
|
Oh yeah, sorry, I'll definitely fix the style before merge. Gotta do some other stuff first, but will get to that later today. Thanks |
|
Thanks, will await another ping before merging. |
|
Pinged on commit. Should be good now |
|
Merge away lad.... |
|
Just testing... |
|
Looking good, I've merged and all seems to work. Good job 🚀 |
|
There are some issues, however, that I've bundled into an issue here: #38 |
|
BTW apologies @mpadge I meant "One for here?: r-lib/styler#340 " definitely a 'for another day' though, and overall this is a huge step forward, thanks for adding the new tab! |
No description provided.