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

James/second tree dropdown #1795

Merged
merged 2 commits into from
Jun 27, 2024
Merged

James/second tree dropdown #1795

merged 2 commits into from
Jun 27, 2024

Conversation

jameshadfield
Copy link
Member

@jameshadfield jameshadfield commented Jun 26, 2024

Don't display the sidebar UI if we're just going to display an empty
dropdown!

See https://bedfordlab.slack.com/archives/C0K3GS3J8/p1719424903252149
for context

  • Checks pass (Our CI passes, RTD is currently broken)
  • If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
  • (to be done by a Nextstrain team member) [Create preview PRs on downstream repositories][1].

The previous code used `.filter` in a rather strange way! There should
be no changes here, although the new code does ensure the displayed
options don't include any duplicates and accounts for situations where
the `secondTreeOptions` key is not defined.
Don't display the sidebar UI if we're just going to display an empty
dropdown!

See <https://bedfordlab.slack.com/archives/C0K3GS3J8/p1719424903252149>
for context
@jameshadfield jameshadfield force-pushed the james/second-tree-dropdown branch from ddd4017 to e626f99 Compare June 27, 2024 00:09
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-james-second-tr-jnwlzr June 27, 2024 00:09 Inactive
@@ -1,5 +1,8 @@
# Changelog

* We no longer show the "second tree" sidebar dropdown when there are no available options. The possible options are defined by [the charon/getAvailable API](https://docs.nextstrain.org/projects/auspice/en/stable/server/api.html) response and as such vary depending on the server in use. ([#1795](https://github.com/nextstrain/auspice/pull/1795))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for noting the possible options can vary depending on the server! Prompted me to review how auspice does it internally with findAvailableSecondTreeOptions and how nextstrain.org uses convertManifestJsonToAvailableDatasetList.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this whole area needs to be rethought at some stage -- the current hierarchical dropdown for changing datasets, the available second trees, how it relates to snapshots etc. There's some design work around but nothing concrete.

Copy link
Contributor

Choose a reason for hiding this comment

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

The previous code used .filter in a rather strange way!

git blame just out of bootcamp Jover from ~5 years ago 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! I assumed it was me! Progress from each of us 😃

@jameshadfield
Copy link
Member Author

Note that if the main tree doesn't have any 2nd tree options but you load a second tree by manipulating the URL then you don't get the dropdown option to remove the 2nd tree. I think this is fine (just manipulate the URL to remove it!), but we can add some UI elements here if needed.

@jameshadfield jameshadfield merged commit 2c59883 into master Jun 27, 2024
25 of 26 checks passed
@jameshadfield jameshadfield deleted the james/second-tree-dropdown branch June 27, 2024 01:04
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.

4 participants