-
Notifications
You must be signed in to change notification settings - Fork 163
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
Measurements improvements #1557
Conversation
Previously, long titles will push the measurements panel into a separate line when viewing in "grid" mode¹. This commit updates the CSS for the title of the Measurements panel so that long titles are truncated with an ellipsis and do not interrupt the layout of the page. I had considered another option to wrap the long title instead of truncating it, but this pushes the panel slightly out of line with the tree panel. ¹ #1452 (comment)
This change is motivated by the measurements hover panel display of titer values that can be 0, but I think this is a good change in general because a 0 value should be a valid value for display.
Extract the function to create the title of the legend from the selected colorBy as a util/colorHelpers function. This is done in anticipation that we want a similar title string for the hover panel of the measurements panel.
This change will allow us to add the color-by attribute as a title to the hover panels for the measurements.
This commit decouples the handling of hover panels in mouseover/mouseout events from the SVG drawing functions. This is done in anticipation that we will be adding the color-by attribute to the hover panel title. With the color-by attribute titles, the handleHover function will be dependent on color-by changes, which we do _not_ want to force SVG redrawing.
For both raw measurements and the color-by means, we use the color-by attribute as the title of the hover panel. This will make it easier to parse what the color means for each element in cases where the legend contains too many colors. Note the overall means' hover panels do not have a title because they are not linked to the color-by attribute.
In cases where the y-axis grouping label is too long, the text is scaled down to fit the available space. This does mean that if the text is extremely long, it can become unreadable. We can improve on this by manually splitting the text into parts that can fit on multiple lines, but there's always limits of the available space so punting that for now.
When the grouping value for the y-axis are reference strains, try to find the color and attribute of the reference strain so that they can be displayed with the y-axis label. Allows for easier correlation between the reference and the displayed measurements.
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.
Nice work @joverlee521! I don't actually use the panel day-to-day, so it'd be good to get 👀 from someone who does as well.
Perhaps beyond this PRs work, but the measurement panel reports dates as decimals rather than converting them to YYYY-MM-DD (both in the y-axis labels and on hover).
handleHover(d, "mean", clientX, clientY); | ||
}) | ||
.on("mouseout", () => handleHover(null)); | ||
.attr("stroke", color); |
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.
With the color-by attribute titles, the handleHover function will be dependent on color-by changes, which we do not want to force SVG redrawing.
Could you elaborate on this a bit more? Changes in color-by will necessitate some degree of SVG redrawing -- at least the lines (mean+sd) / circles (raw). Does this change stop the drawMeasurementsSVG
re-rendering, as this does not actually set the colouring [1], whereas (e.g.) drawMeansForColorBy
will still rerun as required (e.g. when color-by changes)?
[1] If I understand the code, it does run drawMeanAndStandardDeviation
which sets the color to layout.overallMeanColor
, but this just adds the elements to the DOM in a non-visible fashion. We need a second call to drawMeanAndStandardDeviation
to show/color them. Similar for the raw dots.
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.
Does this change stop the drawMeasurementsSVG re-rendering, as this does not actually set the colouring [1], whereas (e.g.) drawMeansForColorBy will still rerun as required (e.g. when color-by changes)?
Yes, this is exactly why I made this change. Previously, handleHover
was directly tied to the redrawing of the entire SVG. Now, it its only tied to the coloring changes so we don't have to worry about unnecessary SVG redrawings.
// If necessary, scale down the text to fit in the available space for the y-Axis labels | ||
// This does mean that if the text is extremely long, it can be unreadable. | ||
// We can improve on this by manually splitting the text into parts that can fit on multiple lines, | ||
// but there's always limits of the available space so punting that for now. |
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.
I think it's worth spending 30 minutes to see if an acceptable solution can be found here. There's heaps of vertical pixel space [1]. Splitting on spaces, hypens, slashes etc should allow a reasonably good result.
[1] at least, there is in the measurements paper's h3n2 dataset I'm using to test. Even when colouring by a unchanging genotype, so there can only ever be one category. Perhaps in some configurations this is different?
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.
Yeah, I didn't want to go too far down the SVG text rabbit hole here...From what I understand, we have to draw the text to get the actual rendered width. So we split the text by spaces/hyphens/slashes, then render each substring to figure out the best configuration to fit the substrings into the allotted width. I can play around with this for a little bit.
Would we want to split reference strain names into multiple lines or would we have to special case them to preserve them in one line?
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.
Let's punt on this - don't want this rabbit hole to hold this up. I'll add it to #1463
More of a future to-do, I think, but here are some suggestions to enhance hovering by calling the
Since I'm here, we currently only colour the y-axis if it's a reference sera (and happens to be present in the tree). But for other groupings we can map them to their corresponding reference sera, and then find the set of colours they represent. So there is a valid colour / set of colours for the other groupings as well. I imagine this'd require a bit of prototyping to get right, but could be an interesting direction. |
Create thicker lines for SD and the y-axis label colorings for easier viewing and hovering.
We need the tree strain colors for all strains regardless of visibility so that we can display the colors for the reference strains in the y-axis label even when the strain is not visible in the tree.
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.
This is awesome, @joverlee521! I really like the tooltips on the mean/std view and the color-by annotation and color bar below each reference strain. Those changes alone improve the usability dramatically!
I understand the challenge (and rabbit hole) of the SVG text scaling. In the original implementation, the text got truncated arbitrarily by length. Now, the issue is that longer strings become unreadably small as in the example below when I group by haplotype:
I like that I can copy and paste the text somewhere else to read it, at least, which feels like an improvement over the previous state. In addition to the small font size, the variation in label sizes makes the y-axis appear buggy or inconsistent in a way you'd want to avoid for a publication figure, for example (assuming you use the Auspice SVG for a paper). Interestingly, the coloring annotation text below the reference strain does not get scaled down, so it can extend into the main plot. The combination of these factors can lead to a slightly unreadable reference strain name and a normal-sized coloring annotation that extends into the plot like below:
I notice that the mean/std horizontal lines appear a bit thicker, which is nice when there are not many groups in the current coloring. When there are more groups, though, the lines get quite tightly packed together. I wish the height of each row could scale with the number of coloring groups, so one could have more space between items at the cost of showing fewer rows at a time on the screen.
I also like the 0 values in the raw data tooltips and the ability to consistently display the measurements panel in grid view with truncated panel titles!
My only major issue from the list above is the readability of the scaled y-axis labels. I don't have a great solution idea other than the same that @jameshadfield mentioned of somehow using the abundant vertical space by wrapping the text. Whatever solution we use for the main y-axis label, though, we should probably apply the same approach to the coloring text annotation that can appear below it.
Description of proposed changes
Small improvements to the measurements panel that includes:
Related issue(s)
Partially addresses #1463