Shiny reactive - Githubissues

ATFutures / upthat

Urban planning and transport health assessment tool

4 stars 1 forks source link

Shiny reactive #35

Closed Robinlovelace closed 4 years ago

Robinlovelace commented 4 years ago

The new tab is there, great work. I would make some changes:

Call it 'Health impacts' not scenarios
Not repeat the UI components that are in the map tab, be dry!
Add some results

Here's what I see so far:

mpadge commented 4 years ago

@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.

mpadge commented 4 years ago

@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!

Robinlovelace commented 4 years ago

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.

mpadge commented 4 years ago

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

Robinlovelace commented 4 years ago

Thanks, will await another ping before merging.

mpadge commented 4 years ago

Pinged on commit. Should be good now

mpadge commented 4 years ago

Merge away lad....

Robinlovelace commented 4 years ago

Just testing...

Robinlovelace commented 4 years ago

Looking good, I've merged and all seems to work. Good job :rocket:

Robinlovelace commented 4 years ago

There are some issues, however, that I've bundled into an issue here: https://github.com/ATFutures/upthat/issues/38

Robinlovelace commented 4 years ago

BTW apologies @mpadge I meant "One for here?: https://github.com/r-lib/styler/issues/340 " definitely a 'for another day' though, and overall this is a huge step forward, thanks for adding the new tab!