auspice icon indicating copy to clipboard operation
auspice copied to clipboard

Sidebar toggle to show labels for all tips in tree

Open jameshadfield opened this issue 4 years ago • 7 comments

Background:

Currently we only display tip labels when the total number of tips in-view is below a certain threshold. This prevents hundreds of tips being displayed on big trees, and allows small trees, or zoomed in sections of trees, to display labels.

(Potentially see #237 for further information)

This issue

There should be a toggle in the sidebar (such as we have for "Show Confidence Intervals") which results in tips being displayed for all visible nodes. It's initial state should be defined by the threshold discussed above.

jameshadfield avatar Mar 04 '20 09:03 jameshadfield

@jameshadfield New to the repo, and i'd be happy to take this on - just thought I would confirm the approach. I mocked this up so far on the zika dataset:

show all labels

Questions:

  • Toggle placement / labeling?
  • Would we show/hide the toggle on certain conditions, such as if all the labels are already in view etc.?
  • Should font size be scaled dynamically to prevent overflow? Granted, this may result in some microscopic font sizes in some cases.

Let me know if there is anything else I missed, thanks!

ntaft avatar Mar 14 '20 16:03 ntaft

Thanks @ntaft ⭐️ Your mock looks along the right directions 👍

  • Placement: It should be directly below the toggle for "Show confidence intervals"
  • Wording: It should read "Display all tip labels"
  • Its shouldn't be hidden when (e.g) we're showing all tip labels (using our current logic for toggling them on/off, which #915 aims to improve). Rather it should update the toggle state in the sidebar. This involves more complicated state management as we have to keep track of whether the toggle has been modified by the user, and if so then we don't apply our existing logic to change this state.
  • Re: "Should font size be scaled dynamically to prevent overflow? Granted, this may result in some microscopic font sizes in some cases." Yes, I would be in favor of exploring this, but there will be many different cases to take care of, as some tip names can be very long. If possible (and you're interested) could you do this on a separate PR?

jameshadfield avatar Mar 14 '20 22:03 jameshadfield

Thanks for the awesome work on this @ntaft! In addition to a text box that can be toggled on or off, can you also add a url option that toggles it on or off? @jameshadfield suggested ?tipLabels=on and ?tipLabels=off?

cassiawag avatar Apr 23 '20 22:04 cassiawag

Sorry for the delay on this - and yes, we should be able to add a url-based toggle as well without too much trouble, sounds like a good idea. Will work on wrapping this up this weekend, thanks!

ntaft avatar Apr 24 '20 13:04 ntaft

+1 for this issue -- we've gotten a lot of requests from public health departments.

@ntaft -- is this something you're still interested in working on, or would it be useful to have someone else help or take this on? No worries either way, just wanted to follow up and update the issue :)

sidneymbell avatar Sep 16 '20 23:09 sidneymbell

Hi there,

I took a look at @ntaft fork. The code works well except there are a few instances when the tips either disappear or the toggle is not reset. I think this is due to the way the phyloTree.labels.updateTipLabels() was modified by ntaft to take in an argument alwaysDisplayTipLabels. There are 4 instances of phyloTree.labels.updateTipLabels() being called throughout the code.

Where I am stuck is knowning if this is the correct approach and if it is how to modify the calls to phyloTree.labels.updateTipLabels() to pass the argument alwaysDisplayTipLabels ( which is connected to state).

To be honest, I am completely new to react-redux and I'm not really sure the current implementation is correct. I think that updateTipLabels() should be left to take no arguments (except dt which is never passed).

To see the changes: https://github.com/ammaraziz/auspice/commit/8016c8c5dc93db0475ffd4517e8127f11619eae2

Thanks,

Ammar

ammaraziz avatar Jan 27 '22 01:01 ammaraziz