Skip to content

Commit

Permalink
Re-implement bucket generation and usage
Browse files Browse the repository at this point in the history 
Restructure the way that buckets are generated and represented. Simplify
what `BucketBar` needs to do with the buckets. Fix a bug in which
anchors can be occasionally included in multiple buckets.

Fixes #397
Fixes #2618
  • Loading branch information
@lyzadanger
lyzadanger committed Oct 20, 2020
1 parent 1628a11 commit ca72ee9
Show file tree
Hide file tree
Showing 5 changed files with 265 additions and 417 deletions.
68 changes: 7 additions & 61 deletions src/annotator/plugin/bucket-bar.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,12 @@ import Delegator from '../delegator';
import scrollIntoView from 'scroll-into-view';

import { setHighlightsFocused } from '../highlighter';
import {
findClosestOffscreenAnchor,
constructPositionPoints,
buildBuckets,
} from '../util/buckets';
import { findClosestOffscreenAnchor, anchorBuckets } from '../util/buckets';

/**
* @typedef {import('../util/buckets').Bucket} Bucket
* @typedef {import('../util/buckets').PositionPoints} PositionPoints
*/

const BUCKET_SIZE = 16; // Regular bucket size
const BUCKET_NAV_SIZE = BUCKET_SIZE + 6; // Bucket plus arrow (up/down)
const BUCKET_TOP_THRESHOLD = 115 + BUCKET_NAV_SIZE; // Toolbar

// Scroll to the next closest anchor off screen in the given direction.
function scrollToClosest(anchors, direction) {
const closest = findClosestOffscreenAnchor(anchors, direction);
Expand Down Expand Up @@ -117,30 +108,7 @@ export default class BucketBar extends Delegator {
}

_update() {
/** @type {PositionPoints} */
const { above, below, points } = constructPositionPoints(
this.annotator.anchors
);

this.buckets = buildBuckets(points);

// Add a bucket to the top of the bar that, when clicked, will scroll up
// to the nearest bucket offscreen above, an upper navigation bucket
// TODO: This should be part of building the buckets
this.buckets.unshift(
{ anchors: [], position: 0 },
{ anchors: above, position: BUCKET_TOP_THRESHOLD - 1 },
{ anchors: [], position: BUCKET_TOP_THRESHOLD }
);

// Add a bucket to the bottom of the bar that, when clicked, will scroll down
// to the nearest bucket offscreen below, a lower navigation bucket
// TODO: This should be part of building the buckets
this.buckets.push(
{ anchors: [], position: window.innerHeight - BUCKET_NAV_SIZE },
{ anchors: below, position: window.innerHeight - BUCKET_NAV_SIZE + 1 },
{ anchors: [], position: window.innerHeight }
);
this.buckets = anchorBuckets(this.annotator.anchors);

// The following affordances attempt to reuse existing DOM elements
// when reconstructing bucket "tabs" to cut down on the number of elements
Expand Down Expand Up @@ -205,17 +173,9 @@ export default class BucketBar extends Delegator {

_buildTabs() {
this.tabs.forEach((tabEl, index) => {
let bucketHeight;
const anchorCount = this.buckets[index].anchors.length;
// Positioning logic currently _relies_ on their being interstitial
// buckets that have no anchors but do have positions. Positioning
// is averaged between this bucket's position and the _next_ bucket's
// position. For now. TODO: Fix this
const pos =
(this.buckets[index].position + this.buckets[index + 1]?.position) / 2;

tabEl.className = 'annotator-bucket-indicator';
tabEl.style.top = `${pos}px`;
tabEl.style.top = `${this.buckets[index].position}px`;
tabEl.style.display = '';

if (anchorCount) {
Expand All @@ -229,34 +189,20 @@ export default class BucketBar extends Delegator {
tabEl.style.display = 'none';
}

if (this.isNavigationBucket(index)) {
bucketHeight = BUCKET_NAV_SIZE;
tabEl.classList.toggle('upper', this.isUpper(index));
tabEl.classList.toggle('lower', this.isLower(index));
} else {
bucketHeight = BUCKET_SIZE;
tabEl.classList.remove('upper');
tabEl.classList.remove('lower');
}

tabEl.style.marginTop = (-1 * bucketHeight) / 2 + 'px';
tabEl.classList.toggle('upper', this.isUpper(index));
tabEl.classList.toggle('lower', this.isLower(index));
});
}

isUpper(i) {
return i === 1;
return i === 0;
}

isLower(i) {
return i === this.buckets.length - 2;
return i === this.buckets.length - 1;
}

isNavigationBucket(i) {
return this.isUpper(i) || this.isLower(i);
}
}

// Export constants
BucketBar.BUCKET_SIZE = BUCKET_SIZE;
BucketBar.BUCKET_NAV_SIZE = BUCKET_NAV_SIZE;
BucketBar.BUCKET_TOP_THRESHOLD = BUCKET_TOP_THRESHOLD;
44 changes: 17 additions & 27 deletions src/annotator/plugin/test/bucket-bar-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,8 @@ describe('BucketBar', () => {
};

fakeBucketUtil = {
anchorBuckets: sinon.stub().returns([]),
findClosestOffscreenAnchor: sinon.stub(),
constructPositionPoints: sinon
.stub()
.returns({ above: [], below: [], points: [] }),
buildBuckets: sinon.stub().returns([]),
};

fakeHighlighter = {
Expand Down Expand Up @@ -105,16 +102,16 @@ describe('BucketBar', () => {
describe('updating buckets', () => {
it('should update buckets when the window is resized', () => {
bucketBar = createBucketBar();
assert.notCalled(fakeBucketUtil.buildBuckets);
assert.notCalled(fakeBucketUtil.anchorBuckets);
window.dispatchEvent(new Event('resize'));
assert.calledOnce(fakeBucketUtil.buildBuckets);
assert.calledOnce(fakeBucketUtil.anchorBuckets);
});

it('should update buckets when the window is scrolled', () => {
bucketBar = createBucketBar();
assert.notCalled(fakeBucketUtil.buildBuckets);
assert.notCalled(fakeBucketUtil.anchorBuckets);
window.dispatchEvent(new Event('scroll'));
assert.calledOnce(fakeBucketUtil.buildBuckets);
assert.calledOnce(fakeBucketUtil.anchorBuckets);
});

context('when scrollables provided', () => {
Expand Down Expand Up @@ -144,11 +141,11 @@ describe('BucketBar', () => {
bucketBar = createBucketBar({
scrollables: ['.scrollable-1', '.scrollable-2'],
});
assert.notCalled(fakeBucketUtil.buildBuckets);
assert.notCalled(fakeBucketUtil.anchorBuckets);
scrollableEls[0].dispatchEvent(new Event('scroll'));
assert.calledOnce(fakeBucketUtil.buildBuckets);
assert.calledOnce(fakeBucketUtil.anchorBuckets);
scrollableEls[1].dispatchEvent(new Event('scroll'));
assert.calledTwice(fakeBucketUtil.buildBuckets);
assert.calledTwice(fakeBucketUtil.anchorBuckets);
});
});

Expand All @@ -165,8 +162,10 @@ describe('BucketBar', () => {
// Create fake anchors and render buckets.
const anchors = [createAnchor()];

fakeBucketUtil.buildBuckets.returns([
fakeBucketUtil.anchorBuckets.returns([
{ anchors: [], position: 137 }, // Upper navigation
{ anchors: [anchors[0]], position: 250 },
{ anchors: [], position: 400 }, // Lower navigation
]);

bucketBar.annotator.anchors = anchors;
Expand Down Expand Up @@ -256,21 +255,17 @@ describe('BucketBar', () => {
];
// These two anchors are considered to be offscreen upwards
fakeAbove = [fakeAnchors[0], fakeAnchors[1]];
fakeBelow = [fakeAnchors[5]];
// These buckets are on-screen
fakeBuckets = [
{ anchors: fakeAbove, position: 137 },
{ anchors: [fakeAnchors[2], fakeAnchors[3]], position: 350 },
{ anchors: [], position: 450 }, // This is an empty bucket
{ anchors: [fakeAnchors[4]], position: 550 },
{ anchors: fakeBelow, position: 600 },
];
// This anchor is offscreen below
fakeBelow = [fakeAnchors[5]];

fakeBucketUtil.constructPositionPoints.returns({
above: fakeAbove,
below: fakeBelow,
points: [],
});
fakeBucketUtil.buildBuckets.returns(fakeBuckets.slice());
fakeBucketUtil.anchorBuckets.returns(fakeBuckets.slice());
});

describe('navigation bucket tabs', () => {
Expand Down Expand Up @@ -302,7 +297,7 @@ describe('BucketBar', () => {

// Resetting this return is necessary to return a fresh array reference
// on next update
fakeBucketUtil.buildBuckets.returns(fakeBuckets.slice());
fakeBucketUtil.anchorBuckets.returns(fakeBuckets.slice());
bucketBar.update();
assert.equal(bucketBar.tabs.length, bucketBar.buckets.length);
assert.notExists(bucketBar.element.querySelector('.extraTab'));
Expand Down Expand Up @@ -370,12 +365,7 @@ describe('BucketBar', () => {
});

it('does not display empty bucket tabs', () => {
fakeBucketUtil.buildBuckets.returns([]);
fakeBucketUtil.constructPositionPoints.returns({
above: [],
below: [],
points: [],
});
fakeBucketUtil.anchorBuckets.returns([]);
bucketBar.update();

const allBuckets = bucketBar.element.querySelectorAll(
Expand Down
Loading

0 comments on commit ca72ee9

Please sign in to comment.