Closed steffilazerte closed 5 years ago
You'll need an update to envreportutils to my forked version (https://github.com/steffilazerte/envreportutils/commit/e27e0c856f89d3d8f94f5be63da8dd8b464d2ad6) that I have yet to create a pull request for (and just pushed this morning).
I'm struggling a bit to decide whether envreportutils
is actually the right place for these files. These functions, while based on the CAAQs functions, are specific to this map. Further, they aren't required by any other repositories (unlike the CAAQS functions which were used by two different indicators in two different repositories).
So that being said, I'm not sure if I should keep them in envreportutils
or not.
Feel free to take a look, but this is definitely a draft at this point!
Okay, I'm liking this version better!
I've opted with a look more like the CAAQS indicators, but let me know if you'd prefer to omit the badge. I also figured out how to force the popup to pan properly on the first time it's opened (a bit of java magic, that I think would also be a good addition to the CAAQs indicators. I added the function the the envreportutils package).
The regional popups are a bit bland, but I'm not sure what else to add/change.
The functions in steffilazerte/envreportutils are actually a bit more generic now, so maybe not a bad place to keep them after all. However, I still haven't gotten around to updated the CAAQS functions, and I'd like to do that before putting in a pull request to bcgov/envreportutils (if at all).
I'm officially ready for review, so let me know what you think!
Thanks for your work on this so far, it is looking great. Unfortunately the pop-ups are not behaving for me (flickering, can really only see the labels)—I am wondering if this is related to the Java? I will see if I can troubleshoot tomorrow.
Some minor comments for discussion:
envreportutils::bc_home_button()
?bcmaps::nr_regions()
with bcmaps::bc_bound()
to remove the ocean?Overall I think it is really close to the vision outlined in the opportunity. @ateucher do you have any opinion re: location of various leaflet-related functions (envreportutils
or analysis repos)? Tx both!
I'll let you take a look at the regional popups properly before I move the title/well information. That would create more white space and wouldn't necessarily increase the information content, so I'm a bit torn. They're not that bad, just not as elegant as the well popups :)
Thanks for the tip, pop-ups rendering fine with line 135 commented out. The map looks great, imo.
Regional Pop-Ups: I think the regional pop-up content looks fine—relatively minimal as you say, but what is in there is informative and easy to digest. I think the bar chart titles are redundant, as the pop-up has the region title already. Maybe we could remove the title and shift the # wells info into a second line in the pop-up titles (e.g. Number of Wells Reported: 10)?
Well Pop-Ups: Looks like a bit more real-estate is needed for the monthly plots—the top, horizontal legend is cut-off. Let's be a bit more specific abt More Info, for example Learn More About the Observation Well or Download the Data:? I like the badge, although it does dominate the window, and I wonder if we have a bit of redundancy with reporting the trend category content generally? It is a lot of reporting content in one modal window, so really open to thoughts about what and how to present this for users.
For the little bc map being off-centre, I believe it's this little bit of css that's pushing it down...
Looks really good Steffi! I also think clipping to the bc boundary would be nice. Also, I agree about giving the top-right hydrgraph a bit more emphasis, and the badge a little less...
Okay sounds good, I'll tweak the formatting and see what I can come up with!
As for the location of leaflet-related functions... I think the ones that are applicable to > 1 map and we can see using them for many other SoE maps (popups_centre()
, set_bc_view_on_close()
, set_bc_view()
, possibly popup_create_row()
and popup_combine_rows()
) can stay in envreportutils. Could probably make the case for the caaqs one to be in there too since they are used in two indicators (and will be more in the future). The other ones (i.e., anything with or _groundwater
in the name can probably go in a leaflet_functions.R
(or something) file in the analysis repo. I think the same goes for the css...?
Yeah, that definitely makes sense!
This looks really nice, well done! One strange thing though - the links to the 'GW Network Map' in the well popups don't work. I get the hand cursor and the url shows up the browser status bar, but clicking doesn't do anything.
It's supposed to open in a new browser window. Is it possible it's being prevented by popup-blocking software?
Full update to use new data from new sources and to streamline some of the processes:
Changes to methodology:
Date related changes:
Rmd file changes:
Things to note: