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

make study view dropdown not refresh unnecessarily when removing a chart #3132

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

adamabeshouse
Copy link
Contributor

image

@adamabeshouse adamabeshouse added bug chore This PR is related to code maintenance and removed chore This PR is related to code maintenance labels Apr 2, 2020
Copy link
Member

@kalletlak kalletlak left a comment

Choose a reason for hiding this comment

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

LGTM. but looks like circleCI is failing

@adamabeshouse adamabeshouse force-pushed the sv-charts-dropdown branch 2 times, most recently from 848e601 to 646f3c0 Compare April 2, 2020 21:08
@alisman
Copy link
Collaborator

alisman commented Apr 3, 2020

@adamabeshouse we still have situation on first load where we show options before frequency has loaded. they are not in appropriate sorting order because it depends on frequency. we need to ask product team whether this is desirable or whether we should just wait until frequency is loaded to show anything (@jjgao). The only reason I can think to show unsorted is because frequency sometimes takes a long time and a user might want to just click something instead of waiting for frequency.

Also, When we click between tabs inside of the charts dropdown, even once all data is loaded, there is still a loader the flashes. Can we investigate why?

@alisman
Copy link
Collaborator

alisman commented Apr 6, 2020

@adamabeshouse i think there's a broken e2e on this one:
chart_in_genomic_tab_can_be_updated - .add_chart

Also, if it's not too much trouble could you center the loading animation and text beneath it? also maybe push it down 100px.

Copy link
Collaborator

@alisman alisman left a comment

Choose a reason for hiding this comment

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

see comment about centering animation/message

@alisman
Copy link
Collaborator

alisman commented Apr 6, 2020

@adamabeshouse also, make sure to rebase because @kalletlak made a fix (just merted) that might possible overlap with yours. i don't think so, but possible.

@adamabeshouse adamabeshouse force-pushed the sv-charts-dropdown branch 2 times, most recently from 387e7b6 to fb0be2c Compare April 6, 2020 21:14
Abeshouse, Adam A./Sloan Kettering Institute added 3 commits April 6, 2020 17:54
…arily when chart removed

Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <abeshoua@mskcc.org>
(1) hide options until frequencies are done loading
(2) dont unmount on hide so that the frequencies are cached and
we dont have reloading

Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <abeshoua@mskcc.org>
Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <abeshoua@mskcc.org>
@adamabeshouse adamabeshouse merged commit 63a28b5 into cBioPortal:master Apr 6, 2020
@adamabeshouse adamabeshouse deleted the sv-charts-dropdown branch April 6, 2020 22:43
inodb pushed a commit to inodb/cbioportal-frontend that referenced this pull request Jan 12, 2022
make study view dropdown not refresh unnecessarily when removing a chart

Former-commit-id: 63a28b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants