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

add hgvsg column and link out to genome nexus #2971

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

leexgh
Copy link
Member

@leexgh leexgh commented Jan 7, 2020

add hgvsg column in mutation table and link out to genome nexus

  • hide by default
  • click hgvsg will go to genome nexus variant page
  • long hgvsg will show ... and the whole hgvsg will show in tooltip
  • compute hgvsg in frontend directly
    image

@leexgh leexgh force-pushed the hgvsg-column branch 2 times, most recently from 4d2147e to b69def0 Compare January 7, 2020 16:46
}
else {
return (
<a href={`https://www.genomenexus.org/variant/${hgvsgData}`} target="_blank" rel="noopener noreferrer">
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets make a helper in urls.ts to make these links. the host should only be in once place in our code

<img
height='16'
width='16'
src={require("./GenomeNexusLogo.png")}
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be best if we accomplished this with css. so, make a

assign it a class and give it a background and dimensions in css.


if (status !== null) {
// show loading circle
if (status === TableCellStatus.LOADING) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's weird that we have a separate case for loading. one would think that would take care of loading state.

Copy link
Member Author

Choose a reason for hiding this comment

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

cBioPortal/cbioportal#6988
issue created about the loading state

@@ -0,0 +1,22 @@
.hgvsg-data a,
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets discuss using SASS for nesting these defs

@@ -232,7 +232,18 @@ export function extractGenomicLocation(mutation: Mutation)
// TODO remove when done refactoring mutation mapper
export function genomicLocationString(genomicLocation: GenomicLocation)
{
return `${genomicLocation.chromosome},${genomicLocation.start},${genomicLocation.end},${genomicLocation.referenceAllele},${genomicLocation.variantAllele}`;
// mapping chromosome X with 23
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be good to have a test for this function

return `${genomicLocation.chromosome},${genomicLocation.start},${genomicLocation.end},${genomicLocation.referenceAllele},${genomicLocation.variantAllele}`;
// mapping chromosome X with 23
let chromosome = "";
if (genomicLocation.chromosome === "X") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should use a switch statement instead of if/else when testing against a single value (genomicLocation.chromosome). Good to get in habit of using switch in these kinds of situations. You could also use a an enum with a default. If genomicLocation.chromosome exists in enum, you use it's value. If not, you use Enum.default.

@leexgh leexgh marked this pull request as ready for review January 14, 2020 16:27
@leexgh
Copy link
Member Author

leexgh commented Jan 14, 2020

@alisman fixed, thanks

@leexgh leexgh requested review from alisman and inodb January 14, 2020 16:33
@leexgh leexgh force-pushed the hgvsg-column branch 3 times, most recently from 93bde23 to 12a6d52 Compare January 20, 2020 17:57
@leexgh
Copy link
Member Author

leexgh commented Jan 20, 2020

@tmazor @schultzn could you take a look and give some feedback on the hgvsg column? this column links out to Genome Nexus variant page
https://deploy-preview-2971--cbioportalfrontend.netlify.com/results/mutations?cancer_study_list=5c8a7d55e4b046111fee2296&case_set_id=all&gene_list=EGFR
CC: @jjgao

@inodb
Copy link
Member

inodb commented Jan 21, 2020

@leexgh as discussed in person: let's hide the hgvsg column for now (make it optional) and do a new PR that adds a tooltip for the protein change column with a link to Genome Nexus.

@leexgh leexgh force-pushed the hgvsg-column branch 2 times, most recently from 35e8694 to f64e237 Compare January 22, 2020 16:52
@inodb
Copy link
Member

inodb commented Jan 22, 2020

@leexgh looks good to me! Thanks!

We discussed to add a tooltip to the Protein Change column that will also link out to Genome Nexus, which will be addressed in a separate PR

Since we decided to hide the hgvsg column, maybe ok to release @jjgao @schultzn @tmazor ?

@leexgh leexgh force-pushed the hgvsg-column branch 2 times, most recently from dc2a773 to d04c31c Compare January 22, 2020 18:47
- hgvsg is shown by default
- the hgvsg will be shorted with ... if the ref or var is too long
@alisman alisman merged commit 6275c82 into cBioPortal:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants