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

Use center rather than top position of anchor for bucket-on-screen test #5526

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

robertknight
Copy link
Member

Use the position of the center of an anchor instead of the top to determine whether it should be counted as off-screen or not. Since buckets in the bucket bar are drawn where the center is, this ensures that on-screen buckets merge into off-screen ones at a natural point, instead of when there is still a gap between the two (for the "above screen" bucket) or after the on-screen bucket has gone below the "below screen" bucket.

Fixes #5525

Use the position of the center of an anchor instead of the top to determine
whether it should be counted as off-screen or not. Since buckets in the bucket
bar are drawn where the center is, this ensures that on-screen buckets merge
into off-screen ones at a natural point, instead of when there is still a gap
between the two (for the "above screen" bucket) or after the on-screen bucket
has gone below the "below screen" bucket.

Fixes #5525
@robertknight robertknight force-pushed the use-center-pos-for-bucket-on-screen-test branch from 859ab77 to fbea415 Compare June 8, 2023 15:09
Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

👍🏼

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #5526 (fbea415) into main (bcd622a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5526   +/-   ##
=======================================
  Coverage   99.25%   99.25%           
=======================================
  Files         238      238           
  Lines        9421     9422    +1     
  Branches     2244     2244           
=======================================
+ Hits         9351     9352    +1     
  Misses         70       70           
Impacted Files Coverage Δ
src/annotator/util/buckets.ts 100.00% <100.00%> (ø)

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

@robertknight
Copy link
Member Author

I'm going to get this merged, but @lyzadanger you might still want to be aware of this change.

@robertknight robertknight merged commit 7d82403 into main Jun 12, 2023
@robertknight robertknight deleted the use-center-pos-for-bucket-on-screen-test branch June 12, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merging of buckets into top/bottom buckets happens at unexpected position
2 participants