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 URL query params #1848

Merged
merged 14 commits into from
Sep 12, 2024
Merged

Measurements URL query params #1848

merged 14 commits into from
Sep 12, 2024

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Sep 9, 2024

Description of proposed changes

Add URL query params for the measurements panel, see docs for new query params.
Examples using community dataset

Also see new query params used in example narrative

Related issue(s)

Resolves #1815

Checklist

Doing this in preparation for adding URL params for measurement controls.

The initial control state is constructed then the URL params update the
state. However, the measurements JSON is loaded after this, so it needs
a way to differentiate the clean slate vs the added URL params.
Changes behavior to only use collection's display default on the initial
load of the page when the control state is undefined. If measurements
URL params are added, then the control state will no longer be undefined
so the collection defaults will not overwrite the URL params.
Prevent errors from invalid collection defaults. Falls back to the app
defaults if there are invalid collection defaults.

Also pulls out access of collection's defaults into a separate function
so that it can be used in subsequent changes to add URL params.
Lays the foundation for measurements URL query params, starting with the
simplest `m_overallMean` to toggle the display of the overall mean.

A series of decisions made in this commit:

1. Keeps conversion between query params and control states in the same
file so that they are less likely to be out of sync.
2. Uses a single dispatch action to create both the new control state
and query params to make sure they are in-sync.
3. If the new control state matches the defaults, then the query param
is removed. This matches the behavior of other query params in Auspice.
Use `m_display` to toggle measurements display between "mean" and "raw"
Use `m_threshold` to toggle whether the thresholds are displayed in
the measurements panel. The query param is ignored and removed if the
collection displayed does not have thresholds.
Use query param `m_groupBy` to specify which group by field to use
in the measurements panel. If it is not one of the collection's
groupings, then the query param will be ignored and removed.
Use query param `mf_<field>=<value>` to specify active filters for
the measurements panel. Invalid fields or values are ignored and
removed.

Multiple values for the same field are expected to give the query param
multiple times with different values, e.g.
`mf_reference_strain=A/Stockholm/18/2011&mf_reference_strain=A/Alabama/5/2010`
This is slightly different than the behavior of the tree filter param,
but I'm following this pattern to avoid the issue described in
<#1846>
Doing this in preparation for adding URL query parameter for defining
collection to display. We need to track the defaultCollectionKey in
Redux state to be able to know when to remove the URL query parameter
if the displayed collection is the default.
@joverlee521

This comment was marked as resolved.

Use query param `m_collection=<collection_key>` to specify which
collection to display in the measurements panel. The display will use
the collection's display defaults unless there are other measurements
query params included in the URL.

If the collection key is the default collection, then the query param
will be removed. If the collection key is invalid, then the query param
will be removed and the default collection will be displayed.
@joverlee521 joverlee521 temporarily deployed to auspice-measurements-ur-0g7jwc September 9, 2024 22:28 Inactive
@joverlee521

This comment was marked as resolved.

Previously, existing control state carried over between narrative
slides so the slide's URL query params were not respected.

This commit resets the measurement's state to the default collection and
the control states to the collection's default control states. In the
special case where the narrative's URL query params define a different
collection with `m_collection`, the measurement and control states are
set to the new collections defaults before other query params are
processed.
@joverlee521 joverlee521 temporarily deployed to auspice-measurements-ur-0g7jwc September 10, 2024 00:29 Inactive
Copy link
Contributor

@huddlej huddlej left a comment

Choose a reason for hiding this comment

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

This worked really well for me in the test build, @joverlee521! I love the new docs and the new query field option names, too. Thank you!

The choice to implement multiple filter values as separate query string entries makes those filters unambiguous. I can see myself forgetting how this works initially and trying to use comma-delimited filter values like the other Auspice filters. For example, the following link works as expected with comma-delimited filters on the tree strains and separate query string keys for the reference strain filters:

https://nextstrain-s-nextstrain-zlos8n.herokuapp.com/community/joverlee521/nextstrain-testing/flu/seasonal/h1n1pdm/ha/09-17?m_collection=egg_HI_raw&m_display=raw&m_overallMean=hide&mf_reference_strain=A/California/7/2009&mf_reference_strain=A/SouthAfrica/3626/2013&mf_source=NIMR-report-Feb2012&mf_source=NIMR-report-Sep2011&p=grid&s=A/California/7/2009,A/SouthAfrica/3626/2013

But this link does not work when I try to comma-delimit (probably not a real verb) the reference strain filters:

https://nextstrain-s-nextstrain-zlos8n.herokuapp.com/community/joverlee521/nextstrain-testing/flu/seasonal/h1n1pdm/ha/09-17?m_collection=egg_HI_raw&m_display=raw&m_overallMean=hide&mf_reference_strain=A/California/7/2009,A/SouthAfrica/3626/2013&mf_source=NIMR-report-Feb2012&mf_source=NIMR-report-Sep2011&p=grid&s=A/California/7/2009,A/SouthAfrica/3626/2013

This is a pretty minor issue, though. Barring any JS-related comments other folks have on this PR, I'd be happy to see this merged and released, so I can start using it for real.

@jameshadfield jameshadfield self-requested a review September 11, 2024 17:14
@joverlee521
Copy link
Contributor Author

@jameshadfield and I walked through the Redux state changes in person.
It's a bit of a mess right now because of a few of reasons

  1. deferred loading of the measurements JSON
  2. the ranking of state to use (url params > collection display defaults > app defaults)
  3. narrative page changes with the same dataset but changes in URL params

I'm planning to merge this as-is tomorrow because it works and the new functionality would be immediately helpful for @huddlej. We can plan to revisit the internal workings on how the Redux state is initialized and managed later.

@joverlee521
Copy link
Contributor Author

@jameshadfield also requested two additional test cases in narratives:

  1. Change coloring and measurements display between slides to check if we run into D3 bug from multiple state changes. Changing slide 5 to slide 6 has no issue because the SVG is redrawn when color changes. I don't think there's any other situation that would make multiple changes to the same SVG element.
  2. Change dataset and measurements URL params between slides to verify that the Redux state is reset and used appropriately. Changing slide 1 to slide 2: The new dataset gets loaded with the new measurements data that uses the correct display dictated by the URL params.

@joverlee521 joverlee521 merged commit de94b72 into master Sep 12, 2024
26 checks passed
@joverlee521 joverlee521 deleted the measurements-url branch September 12, 2024 16:10
@jameshadfield
Copy link
Member

walked through the Redux state changes in person.

Notes:

Prior to this work, we load and parse the main JSON and read URL queries all in one redux step (largely createStateFromQueryOrJSONs). Sidecar JSONs are loaded via subsequent actions and there are no URL queries affecting these - loadMeasurements is the relevant function here. This prioritises rendering of the tree, with the downside that a measurements panel flashes in a second later (example). Narrative slide changes (query changes and/or dataset changes) operate the same way.

This PR parses measurements panel URL queries within createStateFromQueryOrJSONs (although the JSON hasn't yet been parsed), and then reads them from redux state in loadMeasurements. Narrative slide changes where the dataset does change are akin to loading a new page. Narrative slide changes where the dataset doesn't change (but query does) apply these changes to the measurements data within createStateFromQueryOrJSONs (via getCollectionDefaultControl), and this is where it feels like the separation isn't as clean as it could be.

We discussed two alternative approaches:

  1. the measurement URL queries are read within loadMeasurements. This is a nicer separation of concerns however narrative slide changes where the dataset doesn't change complicate this, as we never call loadMeasurements. We also want to process all changes within the same action to avoid out-of-sync rendering when a slide changes.
  2. waiting for the measurements JSON to be fetched and loading everything within createStateFromQueryOrJSONs. The downside here is additional complexity in that function. The upsides are rendering all panels simultaneously and making it easier to one day implement measurements queries which modify the tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add URL parameters for the measurements panel options
4 participants