-
Notifications
You must be signed in to change notification settings - Fork 166
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
add legend to map if tree not displayed #818
Comments
I'm currently coordinating with another developer on the drafting of some mockups for this issue. |
For @jameshadfield and @colinmegill I wanted to present an option with some variants and ask for thoughts. Ian, please feel free to provide any feedback you have as well. For each of these, I'm put the legend in the top right to have a sort of Y-axis symmetry with where the legend is on the tree (and because the animation controls have already occupied the top left). This also leaves room at the bottom for animation timeline in the future. No Padding With Padding Animation Timeline With the Tree Visible |
@ncallaway really nice! I like the idea of a timeline across the bottom a lot. That will work in the case of:
|
@jameshadfield @colinmegill I can start implementing today or tomorrow if the design works. Only thing I need to know is: with or without padding? If with padding, should that padding also be applied to the tree view? |
Brilliant, thanks @ncallaway There is a reason why padding shouldn't be introduced in the tree: the legend sometimes occludes parts of the tree, and we want to work on making this better (e.g. see screenshot of current ncov dataset in divergence view & note that already we can't hover/click on the top-most tips [1]). Your mocks don't show the current color-by name and chevron, which are part of the legend on the tree & should be used for the map also. This title has the effect of adding padding, although there is technically none. This is what I think we should do for the map - that is, no padding but with a title+chevron above the legend entries. My default is no horizontal padding, but I'll leave that up to you as to whatever looks best.
Like this a lot.
This seems sensible. The exception is that if the view is in "full" mode (i.e. tree on top of map) then the map should show a legend, as to view it you've scrolled away from the tree's legend. Happy for this to come later / separate PR as needed. Also note that #923 shifted the legend open/close state into redux, and this could be leveraged (with minor modifications) to keep things in sync. At the very least the beginning open-close state of the map legend should check this value (which can be set via URL query), as it does in the tree. Thanks for all your work here 😄 |
@jameshadfield That sounds good. I'll add the title and chevron when I do the implementation. I'll go with no vertical padding (and likely no horizontal padding to match, but I'll look at both while I implement it). The legend will always be on the map unless the map is displayed side-by-side with the tree view, in which case it will be suppressed. I implemented #923, so I'm familiar with those redux changes, and will make sure to incorporate for the map. One question if we completely share the redux state: should hiding the legend in the tree view also hide the legend on the map view? And vice versa? If so, we'll share the same redux state. If not, they'll each get their own redux legend state, and the URL parameter will initialize both of them. My default assumption is that they should each get their own redux entry, and their legend state should be independent of each other. |
Ahh, so you did! 🤦♂
I'm happy for them to be independent, with the exception that the same URL query should set both (if the query is present, that is). |
It looks like the behavior of adding a legend to the map when the tree is disabled got added, so I'm closing out this old issue. |
Similar to #817, when a map is displayed without a tree, the legend should be displayed to indicate the meaning of colours.
The text was updated successfully, but these errors were encountered: