-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add external links #1763
Add external links #1763
Conversation
12cdab3
to
899e75a
Compare
See <nextstrain/auspice#1763> for context
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.
Just reviewing the UI experience and not the code, but I really dig this new modal interface. The new link at the bottom of the page to "View in other platforms" feels consistent with the "Download data" link and the modal interface behaves just like I'd expect based on having used the data window.
I like how the modal window has the potential to expand to support other platforms besides these two. It's really cool to see how well Taxonium handles the Auspice JSONs even to the point of using the dataset title from the JSON in the display. 💯
I also liked this warning for the MicrobeTrace link that tells the user how many tips there are: "Note that trees with over 500 tips may have trouble loading (this one has 2983)."
Finally, I appreciate that the links open in new windows/tabs, which might seem like an obvious thing to do, but it is reassuring to find I don't lose my place on Nextstrain by clicking a link to view in the other platforms. Your use of the "open in new window" icon next to the "View in other platforms" link primes the user to expect that behavior from the modal window.
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.
Nice, glad this was pretty straightforward! I only left some non-blocking comments.
// Taxonium should work with most nextstrain.org URLs, including /fetch and public /groups | ||
// (checking authn status is not something Auspice should be doing), but it doesn't work | ||
// with /community URLs. | ||
if (pathname.startsWith('/community/')) return false; |
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.
I was curious why community builds don't work in Taxonium.
I can enter a community URL (https://nextstrain.org/community/blab/measurements-panel/flu/seasonal/h3n2/ha) on Taxonium and it loads the community dataset.
Was there one particular community build that didn't work?
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.
The inrb-drc/ebola-nord-kivu build doesn't work in taxonium. I didn't do enough digging and assumed we didn't handle those routes in our {GET + application/json} routing (that's why taxonium doesn't load it, it 404s), but the corresponding {GET + application/json} route for the flu dataset you used did work. My guess is that the bug is related to EBOV being a "default" dataset... but the bug is on us, so I'll open up all community links 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.
Relevant commands, if interested:
200 OK: curl -I -H 'Accept: application/json' https://nextstrain.org/community/blab/measurements-panel/flu/seasonal/h3n2/ha
404 Not Found: curl -I -H 'Accept: application/json' https://nextstrain.org/community/inrb-drc/ebola-nord-kivu
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.
My guess is that the bug is related to EBOV being a "default" dataset...
More likely it's because that repo is using v1 (meta + tree) JSONs
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.
I've updated the PR to no longer filter out /community URLs. I'll leave this thread open as it's got some interesting information in it!
// (checking authn status is not something Auspice should be doing), but it doesn't work | ||
// with /community URLs. | ||
if (pathname.startsWith('/community/')) return false; | ||
if (tangle) return false; |
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.
I think it'd be helpful to have a hint as to why the data source or view settings aren't compatible with the external platform. If we don't want to clutter the modal, maybe a brief console message/warning that says tangle trees aren't supported?
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.
style={Object.assign({}, materialButton, {backgroundColor: "rgba(0,0,0,0)", color: medGrey, margin: 0, padding: 0})} | ||
onClick={() => { this.props.dispatch({ type: SET_MODAL, modal: "linkOut" }); }} | ||
> | ||
<FaExternalLinkAlt /> |
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.
I think this icon is a little misleading because the button actually opens a modal and is not an external link. Maybe add this icon next to the actual links within the modal?
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.
You're good at picking icons - do you have one in mind I should use?
Maybe add this icon next to the actual links within the modal?
I tried this but it felt a little messy so I think I'll keep the modal contents as-is
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.
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.
I think it's fine to just leave out the icon, but if you want one, maybe a cloudshare icon?
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.
FaExternalLinkSquareAlt
might be less associated directly with "new window" while still giving the same general meaning. For Taxonium I use FaShare
or an equivalent (curvy arrow).
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.
I've gone with FaExternalLinkSquareAlt
- thanks for all the suggestions!
P.S. Jover, I liked TbCloudShare
but it would require an update to react-icons so FaExternalLinkSquareAlt
won out!
Just so say this is super cool! Thanks for taking the time to make sense of Taxonium's query parameters. These bidirectional integrations are really great. There's definitely a lot I can improve on the Taxonium side, e.g. with surfacing 404s as seen here - and my parsing of your JSONs could still be more complete! - I hope to make more progress there. This is a great motivation! |
The original design had only one modal so the modal design was wrapped up with the contents shown in the modal. Here we separate them to allow different modals in future commits. There should be no user-facing changes with this change.
Doesn't include any actual links yet, but sets out the interface
899e75a
to
39b4ec6
Compare
See <nextstrain/auspice#1763> for context
@jameshadfield , I am on the MicrobeTrace team at the CDC. Sorry, but herokuapp gave me an error when I tried clicking on the measles genome to test the option to view on MicrobeTrace. It says there was no content there. Could you see what the issue is? Thanks! |
The heroku review app no longer exists since this PR has been merged. The code will be included in the next Auspice release (possibly today) and then it should appear on nextstrain.org some time after that. (Note that the functionality in this PR is only enabled for Auspice running on nextstrain.org, as both Taxonum & Microbetrace get the JSON data via {GET + accept: application/json} requests, and these API routes are not part of Auspice.) |
@jameshadfield , we are eager to try/test this feature. No rush, but just wanted to make sure I am not missing something. nextstrain.org is still showing the Feb version of Auspice. I presume we just wait for the new release? Thanks! |
@ikb6 this has been released as part of Auspice 2.53.0 and this should propagate through to the nextstrain.org server in the coming 24 hours |
Adds Taxonium & MicrobeTrace link-outs to Auspice. Each of those sites allows accessing Nextstrain datasets by using a GET call to the path provided to them with with an
{accept: "application/json"}
header. As such some datasets don't work (if we don't support that route), and private datasets don't work for obvious reasons. But most datasets do work for Taxonium, which is cool! MicrobeTrace seems to only load with trees of 500 tips or less and a warning is added if you have more than this, but this functionality closes https://github.com/nextstrain/private/issues/106.See commit messages for implementation details.
@theosanderson you may be interested in the taxonium link-out, especially the query params I'm using which I've mainly figured out by playing around in Taxonium. And are you OK with us including this on nextstrain.org?!
Testing Taxonium
is a little hard because the site running Auspice must support the appropriate GET request and build Auspice with customisations enabling this functionality. The review app from nextstrain/nextstrain.org#816 works nicely. E.g.
Testing MicrobeTrace
is even harder because (I'm guessing) the
url
query is checked and so only "nextstrain.org" or "next.nextstrain.org" will work, despite the review app site supporting the relevant GET route. Not a big problem if it doesn't work on review apps tho. To test you can do the following (and kind of trust me that it'll work like this when we've merged the PRs):E.g. for MicrobeTrace:
nextstrain-s-james-link-2gqxtm.herokuapp.com
withnextstrain.org
(i.e. to this one)