-
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 paper improvements #1631
Conversation
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.
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.
A couple things I noticed when laying 👀 on this.
label="Show measurement threshold" | ||
label="Show measurement threshold(s)" |
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.
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.)
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.
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.
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:
|
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.
b77f787
to
c8e16a1
Compare
Testing to see if lowering the opacity of the raw measurement circles can help with visualizations of denser areas of the plots.
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)
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.
Related issue(s)
Testing