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

Expression Enrichments: replace MinBoxPlot with BoxScatterPlot #2876

Merged
merged 1 commit into from Nov 20, 2019
Merged

Expression Enrichments: replace MinBoxPlot with BoxScatterPlot #2876

merged 1 commit into from Nov 20, 2019

Conversation

kalletlak
Copy link
Member

@kalletlak kalletlak commented Nov 13, 2019 •
edited

Replace MinBoxPlot with BoxScatterPlot
and
Fix cBioPortal/cbioportal#6791

Enrichments multi group support

Screen Shot 2019-11-19 at 12 19 52 PM

Result page enrichments tab updates

Production
Screen Shot 2019-11-19 at 12 18 07 PM

Updated Plot
Screen Shot 2019-11-19 at 12 19 19 PM

adamabeshouse
adamabeshouse requested changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@adamabeshouse adamabeshouse left a comment

Choose a reason for hiding this comment

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

very cool!! just some comments

@kalletlak kalletlak force-pushed the fix-6791 branch 2 times, most recently from 13ff596 to 4162ead Compare November 14, 2019 16:41
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 14, 2019 16:45 Inactive
@kalletlak kalletlak requested a review from jjgao November 14, 2019 16:45
@jjgao
Copy link
Member

jjgao commented Nov 15, 2019 •
edited

@kalletlak somehow I couldn't get it to work...

image

@kalletlak
Copy link
Member Author

@jjgao works for me. Do you have the url for it?

@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 15, 2019 23:49 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 15, 2019 23:55 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 16, 2019 00:01 Inactive
@jjgao
Copy link
Member

jjgao commented Nov 18, 2019

@kalletlak I was able to test it now. It looks good. Thanks

@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 18, 2019 14:56 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 18, 2019 16:21 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 18, 2019 16:38 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 18, 2019 18:33 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 18, 2019 20:36 Inactive
@kalletlak kalletlak force-pushed the fix-6791 branch 2 times, most recently from e7d3055 to cb2725c Compare November 19, 2019 15:07
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 19, 2019 15:07 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 19, 2019 16:55 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 19, 2019 17:18 Inactive
@alisman
Copy link
Collaborator

alisman commented Nov 19, 2019

@ChrisWakefield i'm sorry i'm doing this code review so late.

@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 20, 2019 15:57 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 20, 2019 16:22 Inactive
adamabeshouse
adamabeshouse requested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@adamabeshouse adamabeshouse left a comment

Choose a reason for hiding this comment

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

I also noticed that in the screenshots the boxplots all have vertical scrollbars, is there a way to get rid of those?

src/pages/groupComparison/ClinicalData.tsx Outdated Show resolved Hide resolved
end-to-end-test/remote/specs/core/groupComparison.screenshot.spec.js Show resolved Hide resolved
src/pages/groupComparison/MRNAEnrichments.tsx Outdated Show resolved Hide resolved
src/pages/resultsView/enrichments/ExpressionEnrichmentsContainer.tsx Outdated Show resolved Hide resolved
src/pages/resultsView/enrichments/ExpressionEnrichmentsContainer.tsx Outdated Show resolved Hide resolved
src/shared/components/plots/BoxScatterPlot.tsx Outdated Show resolved Hide resolved
src/shared/components/plots/BoxScatterPlot.tsx Show resolved Hide resolved
src/shared/components/plots/BoxScatterPlot.tsx Outdated Show resolved Hide resolved
@jjgao jjgao had a problem deploying to cbioportal-f-fix-6791-03ht6dqj November 20, 2019 17:58 Failure
adamabeshouse
adamabeshouse requested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@adamabeshouse adamabeshouse left a comment •
edited

Choose a reason for hiding this comment

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

looks great! just one small code change left

can you also address the issue of the scrollbar appearing in the screenshots? does that happen in the real browser or just the screenshot environment?

src/pages/resultsView/enrichments/ExpressionEnrichmentsBoxPlot.tsx Outdated Show resolved Hide resolved
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 20, 2019 18:27 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-fix-6791-03ht6dqj November 20, 2019 19:01 Inactive
@adamabeshouse adamabeshouse merged commit 16f45de into cBioPortal:master Nov 20, 2019
@inodb inodb added the style tweak To indicate the PR does a minor style tweak label Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamabeshouse adamabeshouse adamabeshouse approved these changes

@alisman alisman Awaiting requested review from alisman

@jjgao jjgao Awaiting requested review from jjgao

Assignees
No one assigned
Labels
style tweak To indicate the PR does a minor style tweak
Projects
None yet
Milestone
No milestone
5 participants
@kalletlak @jjgao @alisman @adamabeshouse @inodb