Rewrite bucket-generating logic #2647
Conversation
| { anchors: below, position: window.innerHeight - BUCKET_NAV_SIZE + 1 }, | ||
| { anchors: [], position: window.innerHeight } | ||
| ); | ||
| this.buckets = anchorBuckets(this.annotator.anchors); |
There was a problem hiding this comment.
Ahhhhh this feels better.
| const anchorCount = this.buckets[index].anchors.length; | ||
| // Positioning logic currently _relies_ on their being interstitial |
There was a problem hiding this comment.
Ahhh, this also feels better.
| }); | ||
| } | ||
|
|
||
| isUpper(i) { | ||
| return i === 1; | ||
| return i === 0; |
There was a problem hiding this comment.
TODO: Make these methods unnecessary
| below: Array.from(belowScreenAnchors), | ||
| points, | ||
| }; | ||
| return anchorPositions; |
There was a problem hiding this comment.
You could write this function as one filter/map/sort chain. I'd be fine with it either way.
lyzadanger
force-pushed
the
refactor-bucketing
branch
from
26cd130 to
1403413
Compare
|
@robertknight I've pushed a commit that integrates your initial feedback, most materially fixing the ickiness of the internal bucket-building function formerly known as |
lyzadanger
force-pushed
the
refactor-bucketing
branch
from
1403413 to
ca72ee9
Compare
Codecov Report
@@ Coverage Diff @@
## master #2647 +/- ##
==========================================
+ Coverage 97.70% 97.72% +0.02%
==========================================
Files 200 200
Lines 7548 7527 -21
Branches 1659 1651 -8
==========================================
- Hits 7375 7356 -19
+ Misses 173 171 -2
Continue to review full report at Codecov.
|
lyzadanger
changed the title
There was a problem hiding this comment.
LGTM. This is a huge improvement over the version on master. I requested some minor changes, after which this is good to merge. Give me a shout if you have a query about any of those.
The existing CSS structure is fiddly to understand due to use of ::before and ::after to create the arrows and the amount of overriding of properties that happens to style the .upper and .lower indicators. At least it is self-contained though and we can revisit that later.
lyzadanger
force-pushed
the
refactor-bucketing
branch
from
ca72ee9 to
7f6d011
Compare
This
draft PR demonstrates an approach to refactoringreimplements the logic used to compute and position anchor buckets in the bucket bar.It does not yet have tests.It is my hope that this re-implementation will be more comprehensible to future devs, provide a clearer separation of concerns and more clearly express the desired outcome of the bucketing logic.
It:
bucketsutil in a way that feels more intuitiveBucketBarto maintain (and export) constantsThe resulting buckets are very close to what the previous algorithm created, minus the knack for sometimes putting anchors into multiple buckets (bug #397).
There's probably one more follow-up pass to rectify a few lingering...interesting features of the code...but this is feeling better...
Fixes #2618
Fixes #397