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

Hardcoded block width of 800px on static blocks #3207

Merged
merged 5 commits into from
Sep 24, 2022
Merged

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Sep 21, 2022

This uses a hardcoded block width of 800px for static blocks

The current method uses the window.innerWidth for the width of static blocks. In the very early days, it was model.width (exactly the width of the view) but was changed to window.innerWidth to avoid the blocks being recalculated when e.g. the drawer opens. This stays with a hardcoded value, but smaller than window.innerWidth, and the appropriate number of 800px are calculated to cover the view width (view's model.width)

The sort of superstituous concern I have with using window.innerWidth is it could be very large for ultra-wide monitors, which would also load a large amount of off-screen data since our staticBlocks cover off-screen areas.

Using a hard coded block width of 800px, we would limit the amount of off-screen data loaded to a more reasonable amount, at the cost of doing more block loading. jbrowse 1 shows us that even small block sizes (jb1's blocks are quite small, no hard number off hand but it's like 200px or something) can be fast (jb1 beats jb2 in some benchmarks in jb2profile)

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 21, 2022
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 21, 2022
@cmdcolin
Copy link
Collaborator Author

Note that changing it to something different than 800 changes a lot of hardcoded stuff in our tests, so changing it to 800 just happens to be convenient since our tests ran with window.innerWidth being 800...

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #3207 (4421462) into main (414d69d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3207   +/-   ##
=======================================
  Coverage   59.50%   59.50%           
=======================================
  Files         670      670           
  Lines       28748    28748           
  Branches     6971     6971           
=======================================
  Hits        17107    17107           
  Misses      11320    11320           
  Partials      321      321           
Impacted Files Coverage Δ
packages/core/util/calculateStaticBlocks.ts 100.00% <100.00%> (ø)
products/jbrowse-web/src/util.ts 48.33% <0.00%> (ø)

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

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 21, 2022
@cmdcolin
Copy link
Collaborator Author

@cmdcolin cmdcolin added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Sep 21, 2022
@cmdcolin
Copy link
Collaborator Author

Tested this on the SKBR3 BAM file, it feels reasonably fast so I don't think we will hurt performance. 2-3 blocks will be onscreen at all times with a standard monitor

@cmdcolin
Copy link
Collaborator Author

this happens to fix #2278 by random chance

@cmdcolin
Copy link
Collaborator Author

maybe can try to merge for now

@cmdcolin cmdcolin merged commit 46d72f9 into main Sep 24, 2022
@cmdcolin cmdcolin deleted the hardcoded_block_width branch September 24, 2022 00:01
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 24, 2022

(the reason it fixed #2278 is because the model.width is now used for the range of blocks to calculate whereas before it used window.innerWidth which is not "observable")

cmdcolin added a commit that referenced this pull request Sep 30, 2022
* Hardcode static block widths

* Updates

* Update snaps

* Add hardcoded width

* Update story to have static block listing
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.

None yet

1 participant