Skip to content
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

Open
jameshadfield opened this issue Oct 30, 2019 · 7 comments
Open

add legend to map if tree not displayed #818

jameshadfield opened this issue Oct 30, 2019 · 7 comments
Assignees
Labels
enhancement New feature or request narratives issues / PRs relating to narrative functionality please take this issue

Comments

@jameshadfield
Copy link
Member

Similar to #817, when a map is displayed without a tree, the legend should be displayed to indicate the meaning of colours.

@jameshadfield jameshadfield added enhancement New feature or request narratives issues / PRs relating to narrative functionality labels Oct 30, 2019
@ncallaway
Copy link

I'm currently coordinating with another developer on the drafting of some mockups for this issue.

@ncallaway
Copy link

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
This option matches the current implementation of the legend on the tree (which also doesn't have padding), but doesn't match with the animation control buttons in the map.

Legend Top Right

With Padding
This option matches the animation controls in the map, but is different from the implementation in the tree. I think this is my preferred option, as I think the padding helps with the legibility of the bounds of the map view (it strengthens the outline instead of breaking it up). If we went with this option, I'd recommend adding the same padding to the legend in the tree view (unless there's a reason we shouldn't).

Legent Top Right w: Padding

Animation Timeline
This just indicates one spot where the animation timeline controls could live when that future issue is implemented.

Legend Top Right w: Timeline Controls

With the Tree Visible
This option just indicates that the map legend will not be visible when the tree view is visible.

Legend Not Displayed

@colinmegill
Copy link
Collaborator

@ncallaway really nice! I like the idea of a timeline across the bottom a lot. That will work in the case of:

  • mobile, when the sidebar is closed
  • narrative, when there is no sidebar or tree to offer a time axis
  • divergence, when tree x axis is mutations

divergence

@ncallaway
Copy link

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

@jameshadfield
Copy link
Member Author

jameshadfield commented Mar 9, 2020

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.

This just indicates one spot where the animation timeline controls could live when that future issue is implemented.

Like this a lot.

This option just indicates that the map legend will not be visible when the tree view is visible.

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 😄


[1] Legend obscuring tips
image

@ncallaway
Copy link

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

@jameshadfield
Copy link
Member Author

I implemented #923, so I'm familiar with those redux changes

Ahh, so you did! 🤦‍♂

should hiding the legend in the tree view also hide the legend on the map view? And vice versa?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request narratives issues / PRs relating to narrative functionality please take this issue
Projects
None yet
Development

No branches or pull requests

3 participants