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

Fix pileup "sort by" setting being lost on zoom level change #3319

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Nov 8, 2022

Fixes #3318

The BaseLinearDisplayModel has a currBpPerPx that collided with the LinearPileupDisplay's currBpPerPx. Renamed to currSortBpPerPx to disambiguate

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Nov 8, 2022
@cmdcolin cmdcolin changed the title Fix sort on zoom Fix pileup "sort by" setting being lost on zoom level change Nov 8, 2022
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Nov 8, 2022
@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #3319 (638bfca) into main (d54632d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3319      +/-   ##
==========================================
+ Coverage   59.44%   59.51%   +0.06%     
==========================================
  Files         736      736              
  Lines       28997    28999       +2     
  Branches     7017     7017              
==========================================
+ Hits        17238    17259      +21     
+ Misses      11513    11494      -19     
  Partials      246      246              
Impacted Files Coverage Δ
...lugins/alignments/src/LinearPileupDisplay/model.ts 66.39% <100.00%> (-0.56%) ⬇️
...s/alignments/src/PileupRenderer/PileupRenderer.tsx 51.01% <0.00%> (-0.61%) ⬇️
products/jbrowse-web/src/util.ts 48.33% <0.00%> (ø)
packages/core/util/layouts/GranularRectLayout.ts 90.90% <0.00%> (+2.16%) ⬆️
plugins/hic/src/HicRenderer/HicRenderer.tsx 100.00% <0.00%> (+4.54%) ⬆️
plugins/alignments/src/PileupRenderer/sortUtil.ts 62.29% <0.00%> (+27.86%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Nov 8, 2022

adds test for the zoom out now also

@cmdcolin cmdcolin merged commit bc6adcb into main Nov 8, 2022
@cmdcolin cmdcolin deleted the fix_sort_zoom branch November 8, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort by pileup setting not maintained if you change zoom level
1 participant