-
Notifications
You must be signed in to change notification settings - Fork 2
v1.1.0 dashboard updates #54
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
…be converted to float.
…rcGIS World Imagery and reduced margins to increase size of graph components, updated tests to look for scattermapbox instead of scattergeo.
Co-authored-by: John Bradley <[email protected]>
Bug fix for lat/lon out of range
… the long change.
…--change carried through all calls and tests). Updated readme to reflect increased flexibility.
Co-authored-by: Matt Thompson <[email protected]>
Recognize `long` and `lon` for longitude, also removes case-sensitivity (sets all columns to capitalized values).
Fix lat/long display in hover-text of map.
Update preview images in Readme to reflect updates on dev branch since v1.0.0 release.
Just to clarify for what there is to review, everything in this change set was already reviewed through PRs previously, right? Or are there previously unreviewed changes that review should be focused on? The change set is a bit big and unwieldy, but if this is just a concatenation of previously reviewed changes, then there's perhaps very little (or even nothing) to review that's new. I.e., trying to figure out what the assignment is for reviewers. |
Numbers 2 and 3 (commit c47c550) were not previously reviewed, but the changes from PRs noted in 4 and 5 were built on top of those changes. |
|
It seems from this discussion that is a desired, but unimplemented feature. I do have the option to reduce the zoom to--most likely--get points on screen. This is the zoom set to 2; it will always produce a map of this size centered on the centroid of the lat/longs provided. Zoom set to 3 is more closely cropped, so cuts off for data with a larger spread, but works nicely for data that is more concentrated. I think it makes more sense to err on the side of being more zoomed out (zoom set to 2), though such a large image suggests all data is caught when it may not be. What do you think? (Also, @hlapp and @thompsonmj?) |
I would argue the default should be to zoom out to a level that all data are shown. I think there's a good point that in terms of UX this may mean to zoom out more than strictly necessary if otherwise the view is sufficiently zoomed in to (in this case falsely) give the impression that only some of the data are shown (when really all are). |
…ering that some data may not be displayed without adjusting zoom.
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.
LGTM
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 all good to me! 👍
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.
Tested and nothing seemed broken! I like the default view level setting.
This update brings minor version developments implemented in dev:
long
as acceptable column name for longitude (PR Recognize long and lon for longitude #51).