-
Notifications
You must be signed in to change notification settings - Fork 45
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
Take liftover into account when switching from v2 to v4 variants #1293
Conversation
closes #1259 |
09a7424
to
772d071
Compare
@phildarnowsky-broad do you have an example variant that shows the disambiguation page? |
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 selector works great. I'm having trouble finding a variant where I encounter the disambiguation page.
I'll note the dataset selector is now inconsistent with other parts of the site. Namely it doesn't show any v3 datasets and if you are v4 you can't see any GRCh37 datasets. I'd be in favor of adding the v2 -> v4 lift over in the selection menu and then making the dataset selector consistent with what users see on the gene page; otherwise it's unclear why some datasets are randomly missing when you're on the variant page.
@mattsolo1 |
@mattsolo1 @ch-kr per conversation in Slack I'm going to give interested parties a chance to argue for or against changing the dataset selector as you describe. |
The screenshot you show is on a GRCh38 variant (so it shoes v4 and v3); my screenshot is from a GRCh37 variant of your new branch (so it shows V4 and V2, but not V3). Currently the browser, when on the variant page, limits what datasets you can select (presumably because liftover was not used by the dataset selector). Now that you've added liftover support to the dataset selector, my point is we can now feel free to add all the datasets as options to the selector. For what it's worth I don't know if people are dying to be able to do this or not, but since you say it is not much effort I don't see why not. It feels better to be able to hop back and forth rather than one direction only. |
@mattsolo1 ah, I think I was misunderstanding you--I thought you were talking about this as something that had worked and was now broken, but this would be new behavior actually? if so, makes sense to me: I'll add in the reverse liftover and remove it if anyone really doesn't like it. |
I slightly prefer the lifting in a single direction only to push users to move from b37 to b38, though I guess we should support the other direction if not having liftunder from 38 back to 37 is really impacting clinical pipelines |
@ch-kr I don't think we should choose not add low hanging fruit functionality for that reason. We don't know why they may want to lift back and forth between new and old variants. Perhaps they are trying to cross reference new literature with old literature, etc. |
that's a good point! works for me |
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.
Very clean and nice code quality. Loving all the tests.
Just one requested change to pass a param that should fix broken links observed when the variant disambiguation page gets used.
<Link to={`/variant/${variantId}?dataset=${datasetToLink}`}> | ||
{variantId} | ||
</Link> |
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.
When I was manually checking here on the multiple variant disambiguation page (set the conditionals above to always be false so this branch triggers) these links are broken for me, the Link
appends the new dataset query param on top of the v2 dataset from where it came from (i.e. directed to "http://localhost:8008/variant/1-55039779-T-G?dataset=gnomad_r4?dataset=gnomad_r2_1"), this double query param results in an error page.
I believe the Link
component has a default value for a prop called preserveSelectedDataset
as true, to keep the dataset query param when navigating between pages.
gnomad-browser/browser/src/Link.tsx
Lines 22 to 45 in d9085aa
if (preserveSelectedDataset) { | |
const currentParams = queryString.parse(location.search) | |
if (typeof to === 'string') { | |
finalTo = { pathname: to, search: queryString.stringify({ dataset: currentParams.dataset }) } | |
} else { | |
const toParams = queryString.parse(to.search) | |
finalTo = { | |
...to, | |
search: queryString.stringify({ ...toParams, dataset: currentParams.dataset }), | |
} | |
} | |
} | |
// @ts-expect-error TS(2769) FIXME: No overload matches this call. | |
return <StyledRRLink {...rest} to={finalTo} /> | |
}) | |
Link.propTypes = { | |
preserveSelectedDataset: PropTypes.bool, | |
} | |
Link.defaultProps = { | |
preserveSelectedDataset: true, | |
} |
As such, just passing false to this will fix the link.
<Link
to={`/variant/${variantId}?dataset=${datasetToLink}`}
preserveSelectedDataset={false}
>
{variantId}
</Link>
772d071
to
8bdfe9f
Compare
@mattsolo1 added a small commit to switch on the reverse liftover, I'm looking currently for a multiple-variant liftover and will post a link when I find one |
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.
LGTM, with squashes of fixup commits of course.
a58627e
to
1696ea1
Compare
This adds a disambiguation page for liftover, similar to the existing one for RSIDs, since as with RSIDs, a v2 variant may lift over to multiple v4 variants, and vice versa. Switching to v4 via the dataset selector on the variants page will link to this new disambiguation page.