-
Notifications
You must be signed in to change notification settings - Fork 168
New dataset selector #2030
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
base: master
Are you sure you want to change the base?
New dataset selector #2030
Conversation
|
Nice! I found this nested example useful for exploring more: https://nextstrain-s-nextstrain-amsecf.herokuapp.com/groups/blab/flu/seasonal/h1n1pdm/ha/12y Haven't looked at the code, but looks great in terms of functionality. Not sure if you want feedback on UI at this stage, so I'll hold back on that. |
|
Nice, I really like this direction! In brief testing, this works with core, groups (within single group), and community (within single repo). Doesn't work as well with staging but that seems expected since the available datasets for staging is just a duplication of core. I'm all for pushing on this to polish the UI and getting this out! |
Great - seems like there's consensus here. I think the behaviour is pretty good, although I still want to:
Good observation. I think that should be improved, but on the server-side as you imply. The simplest would be akin to what we do for groups (basically run
Please share any big UI ideas you have! I was thinking of adding buttons/tabs for "datasets" vs "narratives", but otherwise just polishing what you see here. |
Not used for the current modals but will be for the upcoming dataset selector
b64507c to
97db69f
Compare
The sidebar-based dataset selector has existed unchanged for many years now, despite many nextstrain.org changes to how datasets are served (different sources, snapshots etc). This commit completely redesigns the selector UI, whilst still leveraging the same underlying data (via the `charon/getAvailable` API call). The two big UI improvements are to allow multiple changes to different parts of the heirarcy (rather than changing page as soon as anything was changed), and to allow snapshots to be requested. A number of bugs are fixed, namely those which occured when changing an intermediate-level option wouldn't preserve deeper options.² For instance, previously viewing "seasonal-flu/h3n2/na/3y" then changing "na" to "ha" would change "3y" to "2y"¹. The UI now preserves the choices for different levels where possible, and selects defaults where necessary.³ As with a redesign like this there are future avenues to persue if we choose to! * There is a conflict between Auspice-specific code and nextstrain.org specific code, especially for snapshots. * Snapshots aren't available in the (nextstrain.org) `getAvailable` API response, so we allow any YYYY-MM-DD value and rely on the server to handle these appropriately. * The nextstrain.org `getAvailable` data is per-source, e.g. for core datasets you can only see other core datasets. We could greatly rethink this, but this is better done as a redesign of the getAvailable API. * Expose available narratives via the UI. I started out intending to do this but didn't get round to it. Closes <#560> Closes <#697> Closes <nextstrain/nextstrain.org#352> Closes <nextstrain/nextstrain.org#40> ¹ This happened because the client made a request to `seasonal-flu/h3n2/ha` which was expanded to the canonical/default 2y timespan ² This resulted in bugs when the server didn't redirect, most common on nextstrain.org groups and community pages, e.g. see <nextstrain/nextstrain.org#40> ³ If a level's chosen option isn't available (e.g. change 'seasonal-flu' to 'avian-flu' and thus 'h3n2' doesn't exist) then we choose a default by looking at the first applicable option from the available datasets. This doesn't necesarily match the default on the server-side, which is set via the manifest JSON. In the avian-flu case, the UI will select 'h5n1-cattle-outbreak' but the server would have selected 'h5n1'.
97db69f to
c43cec4
Compare
joverlee521
left a comment
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.
Left mostly minor comments and notes on odd behaviors I noticed. Overall, like I said before, I really like this new design!
| <ChangeDatasetRow id={SIDEBAR_DATASET_CHANGE_ID}> | ||
| <ChangeDatasetLabel> | ||
| <SearchIcon /> | ||
| {this.props.t('sidebar:changeDataset', 'change dataset')} |
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 nit
Instead of providing a default value, I'd prefer to add the text to src/locales/en/sidebar.json so that it's easy to pick up translations later. Also, looks like we usually use the raw text display as the key
| {this.props.t('sidebar:changeDataset', 'change dataset')} | |
| {this.props.t('sidebar:change dataset')} |
|
|
||
|
|
||
| override componentDidMount(): void { | ||
| Mousetrap.bind('enter', this.changeDataset); |
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
Minor annoyance with Mousetrap.bind is that the keyboard event will only fire when the focus is outside of an input. So after changing one of the dataset selects, the user has to click/tab to unfocus the input then press 'enter' to load the new dataset.
I don't think there's much we can do here, but wanted to flag in case others have ideas.
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 actually, pressing 'enter' does not work as expected for me, it just reloads the same dataset:
Screen.Recording.2026-01-09.at.2.32.18.PM.mov
| // highlight (red) if changed | ||
| const style = matchAgainst && matchAgainst.parts[idx]!==word ? {color: 'orange'} : {} | ||
| return [<Strong style={style} key={word}>{word}</Strong>, idx+1===dataset.parts.length ? null : <> / </>] | ||
| }) | ||
| if (dataset.snapshot) { | ||
| jsx.push(<> @ </>) | ||
| const style = matchAgainst && matchAgainst.snapshot!==dataset.snapshot ? {color: 'red'} : {} |
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.
Did you mean to use different colors for the dataset parts (orange) vs the snapshot (red)?
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, but open to better ideas! I wanted red to convey the snapshot is invalid, and orange to indicate which parts of the proposed dataset are different.
| /** | ||
| * Render a freeform text input for snapshot name, styled to match CustomSelect. | ||
| * | ||
| * A note on snapshots: In the vanilla Auspice client/server a '@YYYY-MM-DD' part of the dataset | ||
| * isn't special, it needs to be part of the actual filename to work; this also means the available | ||
| * "snapshots" are known to us via this.props.available. In the nextstrain.org server it's special | ||
| * cased, and if it's not in YYYY-MM-DD format then loading that dataset results in a Bad Request. | ||
| * | ||
| * This implementation is catered for nextstrain.org usage -- you can enter in any datestamp but | ||
| * we enforce the YYYY-MM-DD format client-side. So if you use `@` in your datasets in Auspice | ||
| * functionality of the dataset selector will be less than idea. Long term, I think we should | ||
| * incorporate the idea of special-casing '@' in Auspice too! | ||
| */ |
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.
Not asking for change, just want to document behavior that I've seen
Really awesome that this works for snapshots of the core datasets in nextstrain.org! I wish we could hide it for groups/community which don't support the snapshots. From a groups/community dataset, entering a snapshot will go to an error page, which I think is ~fine.
As you noted this is not ideal in vanilla Auspice: if I enter a snapshot that does not exist as part of the filename, then the dataset goes to the default (?) dataset instead of an error page.
Screen.Recording.2026-01-09.at.2.55.50.PM.mov
| tabIndex={0} | ||
| aria-label="Change dataset" | ||
| onClick={(): void => { | ||
| this.props.dispatch({ type: SET_MODAL, modal: "datasetSelector" }) |
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
Will note that using the modal for changing datasets is not ideal in mobile view but don't want to block this on that.
Review app for nextstrain.org
Review app for stand-alone Auspice
The sidebar-based dataset selector has existed unchanged for many years
now, despite many nextstrain.org changes to how datasets are served
(different sources, snapshots etc).
This commit completely redesigns the selector UI, whilst still
leveraging the same underlying data (via the
charon/getAvailableAPIcall). The two big UI improvements are to allow multiple changes to
different parts of the heirarcy (rather than changing page as soon as
anything was changed), and to allow snapshots to be requested.
A number of bugs are fixed, namely those which occured when changing an
intermediate-level option wouldn't preserve deeper options.² For
instance, previously viewing "seasonal-flu/h3n2/na/3y" then changing
"na" to "ha" would change "3y" to "2y"¹. The UI now preserves the
choices for different levels where possible, and selects defaults where
necessary.³
As with a redesign like this there are future avenues to persue if we
choose to!
specific code, especially for snapshots.
getAvailableAPIresponse, so we allow any YYYY-MM-DD value and rely on the server to
handle these appropriately.
getAvailabledata is per-source, e.g. for coredatasets you can only see other core datasets. We could greatly
rethink this, but this is better done as a redesign of the
getAvailable API.
this but didn't get round to it.
Closes #560
Closes #697
Closes nextstrain/nextstrain.org#352
Closes nextstrain/nextstrain.org#40
¹ This happened because the client made a request to
seasonal-flu/h3n2/hawhich was expanded to the canonical/default 2ytimespan
² This resulted in bugs when the server didn't redirect, most common
on nextstrain.org groups and community pages, e.g. see
nextstrain/nextstrain.org#40
³ If a level's chosen option isn't available (e.g. change 'seasonal-flu'
to 'avian-flu' and thus 'h3n2' doesn't exist) then we choose a default
by looking at the first applicable option from the available datasets.
This doesn't necesarily match the default on the server-side, which is
set via the manifest JSON. In the avian-flu case, the UI will select
'h5n1-cattle-outbreak' but the server would have selected 'h5n1'.
Original PR message (now outdated)
As part of diving into the quirks of how we change datasets yesterday I ended up prototyping how a new auspice selector might work. Early days, but I think this has promise. The main idea is that we leverage the
getAvailabledata API to limit Auspice into only allowing valid selections, so there should never be a situation where the server has to canonicalize / redirect the requested dataset.@victorlin @joverlee521 I'd be interested in your thoughts at this early stage
E.g. load https://nextstrain-s-nextstrain-amsecf.herokuapp.com/groups/blab/229e/E, click on the dataset (top of sidebar), and interact with the new modal.