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

Measurements paper improvements #1631

Merged
merged 5 commits into from
Feb 7, 2023
Merged

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Feb 3, 2023

Description of proposed changes

Minor improvements for the measurements panel that addresses review for the measurements panel manuscript, which also align with improvements listed in #1463.

  • sticky x-axis
  • multiple thresholds
  • group raw measurements by color-by values

Related issue(s)

Testing

Uses "sticky" positioning to make sure the x-axis is always visible even
when the SVG overflows the container. The main SVG's position has been
changed to "relative" to shift up the bottom based on the x-axis height.
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-measurements-pa-dtidxy February 3, 2023 01:16 Inactive
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things I noticed when laying 👀 on this.

Comment on lines -117 to +120
label="Show measurement threshold"
label="Show measurement threshold(s)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking

This diff made me realize two things.

Labels like this should go thru our i18n framework (i18next) so they can be translated in the future.

label={t("Show measurement threshold(s)")}

Once they do, we can avoid the ugly "(s)" by using the standard pluralization/number agreement features, e.g. something like:

label={t("Show measurement threshold", {count: collection.thresholds.length})}

with corresponding translations, e.g. for en something like:

{
  "Show measurement threshold_one": "Show measurement threshold",
  "Show measurement threshold_others": "Show measurement thresholds",
}

(I realize these are out of scope for this PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I'm totally missing the translation feature for all measurements options...

Very cool that i18next supports plurals formatting like this!
I decided that I'm going to punt this for now as I was reading the i18next docs since we are still using i18next v19.9.2 and I haven't dug into which JSON format we are using:

Starting with i18next>=21.3.0 you can use the built-in formatting functions based on the Intl API

You may need to polyfill the Intl.PluralRules API, in case it is not available it will fallback to the i18next JSON format v3 plural handling.

src/components/measurements/measurementsD3.js Outdated Show resolved Hide resolved
@joverlee521
Copy link
Contributor Author

joverlee521 commented Feb 3, 2023

EDIT: Fixed this issue in e2794b5

Saw a weird behavior with the sticky x-axis in the test app. Non-blocking, but would be good to investigate:

  1. Scroll to the bottom of the measurements panel.
  2. Change filtering/options that shortens the main SVG, e.g. change "Group By" from "reference_strain" to "source".
  3. See the x-axis is stuck at the bottom with a bunch of white space above.
  4. Scrolling back up to the plots will fix the issue.

Screen Shot 2023-02-03 at 11 25 19 AM
Screen Shot 2023-02-03 at 11 25 30 AM
Screen Shot 2023-02-03 at 11 26 19 AM

The measurements panel now expects each collection to have an array of
threshold values.

This change is backwards compatible because single value thresholds are
converted to an array during the loading of the measurements JSON.
Note `collection.thresholds` takes precedence over the deprecated
`collection.threshold`, so the single value will be ignored if
`collection.thresholds` exists.

All threshold values will be displayed as solid grey lines across the
subplots. The toggle for showing thresholds applies to all threshold
values. Future improvements include allowing customizations of
solid vs dashed lines and individual toggles for each threshold.
For each subplot, jitter the y-values of the raw measurements within
each color-by group. The y-value range for each color-by group is
determined by the proportion of measurements within each group, i.e.
color-by groups with more measurements get a larger portion of the
subplot height.

Removes the jitter value added during loading of the measurements JSON
and the `measurementJitterSymbol` since they are no longer needed.
@joverlee521 joverlee521 force-pushed the measurements-paper-improvements branch from b77f787 to c8e16a1 Compare February 3, 2023 20:47
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-measurements-pa-dtidxy February 3, 2023 20:48 Inactive
Testing to see if lowering the opacity of the raw measurement circles
can help with visualizations of denser areas of the plots.
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-measurements-pa-dtidxy February 3, 2023 21:18 Inactive
Restores the behavior prior to adding the sticky x-axis where the
container resets to the top when the SVG is redrawn.

This explicit reset fixes the issue where the sticky x-axis can
potentially leave the panel in a scrolled state that displays a
huge white space.¹

¹ #1631 (comment)
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-measurements-pa-dtidxy February 3, 2023 23:35 Inactive
@joverlee521 joverlee521 merged commit 15f304f into master Feb 7, 2023
@joverlee521 joverlee521 deleted the measurements-paper-improvements branch February 7, 2023 18:29
@joverlee521 joverlee521 mentioned this pull request Feb 8, 2023
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants