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.

Sign up for GitHub

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

Jump to bottom

react-variant-view update with existing signal data #3489

Merged
merged 2 commits into from
Dec 7, 2020
Merged

react-variant-view update with existing signal data #3489

merged 2 commits into from
Dec 7, 2020

Conversation

leexgh
Copy link
Member

@leexgh leexgh commented Nov 3, 2020 •
edited

@leexgh leexgh changed the title react-variant-view update for Pathogenicity (DO NOT MERGE) react-variant-view update (DO NOT MERGE) Nov 5, 2020
@leexgh leexgh changed the title react-variant-view update (DO NOT MERGE) react-variant-view update with existing signal data Nov 10, 2020
@leexgh leexgh marked this pull request as ready for review November 10, 2020 15:59
@leexgh leexgh force-pushed the react-variant-view-update branch 3 times, most recently from 81d9fb4 to a7a9790 Compare November 13, 2020 03:18
@leexgh
Copy link
Member Author

leexgh commented Nov 13, 2020

I copied over some files from signal, those files have a comment on the top: // this component comes from signal. We could skip those files when review. Later we can refactor those copied files somewhere

onursumer
onursumer requested changes Nov 18, 2020
View reviewed changes
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

I think we should try to refactor/relocate duplicates before merging this PR

packages/react-variant-view/src/component/cancerPatientPopulation/CancerPatientPrevalence.tsx Outdated Show resolved Hide resolved
packages/react-variant-view/src/component/frequencyTable/FrequencyCell.tsx Outdated Show resolved Hide resolved
packages/react-variant-view/src/component/frequencyTable/MutationTumorTypeFrequencyTable.tsx Outdated Show resolved Hide resolved
...react-variant-view/src/component/generalPopulationPrevalence/GeneralPopulationPrevalence.tsx Outdated Show resolved Hide resolved
packages/react-variant-view/src/component/pathogenicity/ClinVarInterpretation.tsx Show resolved Hide resolved
packages/react-variant-view/src/util/FilterUtils.ts Outdated Show resolved Hide resolved
packages/react-variant-view/src/util/MutationDataUtils.ts Outdated Show resolved Hide resolved
packages/react-variant-view/src/model/GeneFrequencySummary.ts Outdated Show resolved Hide resolved
packages/react-variant-view/src/model/Mutation.ts Outdated Show resolved Hide resolved
onursumer
onursumer requested changes Dec 2, 2020
View reviewed changes
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

Thanks for moving the duplicate code under appropriate packages! Just added minor suggestions regarding naming and location of the functions/models.

packages/cbioportal-utils/src/model/SignalMutationStatus.ts Outdated Show resolved Hide resolved
packages/cbioportal-utils/src/signal/DisplayUtils.ts Outdated Show resolved Hide resolved
packages/cbioportal-utils/src/signal/GeneFrequencySummary.ts Outdated Show resolved Hide resolved
@leexgh leexgh force-pushed the react-variant-view-update branch 6 times, most recently from fbce8e0 to a82710f Compare December 3, 2020 17:31
@leexgh leexgh requested a review from onursumer December 3, 2020 17:34
onursumer
onursumer requested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few minor requests.

packages/cbioportal-utils/src/format/FormatUtils.ts Outdated Show resolved Hide resolved
packages/cbioportal-utils/src/index.tsx Outdated Show resolved Hide resolved
packages/cbioportal-utils/src/index.tsx Outdated Show resolved Hide resolved
packages/react-variant-view/src/component/frequencyTable/FrequencyTable.css Outdated Show resolved Hide resolved
@leexgh leexgh requested a review from onursumer December 3, 2020 18:12
onursumer
onursumer approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@leexgh
update react-variant-view with existing signal data
3cdbcc1
@leexgh
move duplicated functions to other packages
2881791 
- frequencyCell.txs to cbioportal-frontend-commons
- all utils and models go to cbioportal-utils
- MutationTumorTypeFrequencyTable stays at variant-view package, but export for signal
- some other changes to fix comments
@inodb inodb added the cl-package Improvements related to a package label Dec 7, 2020
@inodb inodb merged commit 629d295 into cBioPortal:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inodb inodb inodb approved these changes

@onursumer onursumer onursumer approved these changes

Assignees
No one assigned
Labels
cl-package Improvements related to a package
Projects
None yet
Milestone
No milestone
3 participants
@leexgh @inodb @onursumer