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

Update: Scroll into view #906

Merged
merged 6 commits into from
Feb 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/lib/ThumbnailsSidebar.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isFinite from 'lodash/isFinite';
import VirtualScroller from './VirtualScroller';
import { CLASS_HIDDEN } from './constants';

const CLASS_BOX_PREVIEW_THUMBNAIL = 'bp-thumbnail';
const CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE = 'bp-thumbnail-image';
Expand Down Expand Up @@ -137,6 +138,7 @@ class ThumbnailsSidebar {
const scaledViewport = page.getViewport(this.scale);

this.virtualScroller.init({
initialRowIndex: this.currentPage - 1,
totalItems: this.pdfViewer.pagesCount,
itemHeight: scaledViewport.height,
containerHeight: this.anchorEl.parentNode.clientHeight,
Expand Down Expand Up @@ -342,6 +344,7 @@ class ThumbnailsSidebar {
if (parsedPageNumber >= 1 && parsedPageNumber <= this.pdfViewer.pagesCount) {
this.currentPage = parsedPageNumber;
this.applyCurrentPageSelection();
this.virtualScroller.scrollIntoView(parsedPageNumber - 1);
}
}

Expand All @@ -361,6 +364,56 @@ class ThumbnailsSidebar {
}
});
}

/**
* Toggles the thumbnails sidebar
* @return {void}
*/
toggle() {
if (!this.anchorEl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're already checking for anchorEl in each function below you could remove this , if you want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can because if there is no anchorEl, then isOpen will return false and it will attempt to invoke toggleOpen

return;
}

if (!this.isOpen()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably just use classList.toggle

this.toggleOpen();
} else {
this.toggleClose();
}
}

/**
* Returns whether the sidebar is open or not
* @return {boolean} true if the sidebar is open, false if not
*/
isOpen() {
return this.anchorEl && !this.anchorEl.classList.contains(CLASS_HIDDEN);
}

/**
* Toggles the sidebar open. This will scroll the current page into view
* @return {void}
*/
toggleOpen() {
if (!this.anchorEl) {
return;
}

this.anchorEl.classList.remove(CLASS_HIDDEN);

this.virtualScroller.scrollIntoView(this.currentPage - 1);
}

/**
* Toggles the sidebar closed
* @return {void}
*/
toggleClose() {
if (!this.anchorEl) {
return;
}

this.anchorEl.classList.add(CLASS_HIDDEN);
}
}

export default ThumbnailsSidebar;
57 changes: 55 additions & 2 deletions src/lib/VirtualScroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,12 @@ class VirtualScroller {
this.previousScrollTop = 0;

this.createListElement = this.createListElement.bind(this);
this.isVisible = this.isVisible.bind(this);
this.onScrollEndHandler = this.onScrollEndHandler.bind(this);
this.onScrollHandler = this.onScrollHandler.bind(this);
this.getCurrentListInfo = this.getCurrentListInfo.bind(this);
this.renderItems = this.renderItems.bind(this);
this.scrollIntoView = this.scrollIntoView.bind(this);

this.debouncedOnScrollEndHandler = debounce(this.onScrollEndHandler, DEBOUNCE_SCROLL_THRESHOLD);
this.throttledOnScrollHandler = throttle(this.onScrollHandler, THROTTLE_SCROLL_THRESHOLD);
Expand Down Expand Up @@ -110,7 +112,8 @@ class VirtualScroller {
this.scrollingEl.appendChild(this.listEl);
this.anchorEl.appendChild(this.scrollingEl);

this.renderItems();
// If initialRowIndex is < the first window into the list, then just render from the first item
this.renderItems(config.initialRowIndex < this.maxRenderedItems ? 0 : config.initialRowIndex);

this.bindDOMListeners();

Expand Down Expand Up @@ -245,7 +248,10 @@ class VirtualScroller {
return;
}

let newStartOffset = offset;
// If specified offset is in the last window into the list then
// render that last window instead of starting at that offset
const lastWindowOffset = this.totalItems - this.maxRenderedItems;
let newStartOffset = offset > lastWindowOffset ? lastWindowOffset : offset;
let newEndOffset = offset + this.maxRenderedItems;
// If the default count of items to render exceeds the totalItems count
// then just render up to the end
Expand Down Expand Up @@ -372,6 +378,53 @@ class VirtualScroller {
newListEl.style.height = `${this.totalItems * (this.itemHeight + this.margin) + this.margin}px`;
return newListEl;
}

/**
* Scrolls the provided row index into view.
* @param {number} rowIndex - the index of the row in the overall list
* @return {void}
*/
scrollIntoView(rowIndex) {
if (!this.scrollingEl || rowIndex < 0 || rowIndex >= this.totalItems) {
return;
}

// See if the list item indexed by `rowIndex` is already present
const foundItem = Array.prototype.slice.call(this.listEl.children).find((listItem) => {
const { bpVsRowIndex } = listItem.dataset;
const parsedRowIndex = parseInt(bpVsRowIndex, 10);
return parsedRowIndex === rowIndex;
});

if (foundItem) {
// If it is already present and visible, do nothing, but if not visible
// then scroll it into view
if (!this.isVisible(foundItem)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it that bad to call scrollIntoView even the thumbnail is already visible? that would allow you to get rid of isVisible and it could be kind of nice to top align the thumbnail even if it's already visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not necessarily bad, but since cross browser support for the "smooth" scrollIntoView is not there, I was trying to avoid the slight jumpiness it causes IMO. I can show you tomorrow to see what you think

foundItem.scrollIntoView();
}
} else {
// If it is not present, then adjust the scrollTop so that the list item
// will get rendered.
const topPosition = (this.itemHeight + this.margin) * rowIndex;
this.scrollingEl.scrollTop = topPosition;
}
}

/**
* Checks to see whether the provided list item element is currently visible
* @param {HTMLElement} listItemEl - the list item elment
* @return {boolean} Returns true if the list item is visible, false otherwise
*/
isVisible(listItemEl) {
if (!this.scrollingEl || !listItemEl) {
return false;
}

const { scrollTop } = this.scrollingEl;
const { offsetTop } = listItemEl;

return scrollTop <= offsetTop && offsetTop <= scrollTop + this.containerHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put a () around scrollTop + this.containerHeight? Reading this I saw
scrollTop <= offsetTop && offsetTop <= scrollTop first 😮

}
}

export default VirtualScroller;
101 changes: 99 additions & 2 deletions src/lib/__tests__/ThumbnailsSidebar-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable no-unused-expressions */
import ThumbnailsSidebar from '../ThumbnailsSidebar';
import VirtualScroller from '../VirtualScroller';
import { CLASS_HIDDEN } from '../constants';

const sandbox = sinon.sandbox.create();

Expand All @@ -11,6 +12,7 @@ describe('ThumbnailsSidebar', () => {
let page;
let virtualScroller;
let pagePromise;
let anchorEl;

before(() => fixture.setBase('src/lib'));

Expand All @@ -31,9 +33,11 @@ describe('ThumbnailsSidebar', () => {
stubs.getPage = sandbox.stub().returns(pagePromise);
stubs.vsInit = sandbox.stub(VirtualScroller.prototype, 'init');
stubs.vsDestroy = sandbox.stub(VirtualScroller.prototype, 'destroy');
stubs.vsScrollIntoView = sandbox.stub(VirtualScroller.prototype, 'scrollIntoView');

virtualScroller = {
destroy: stubs.vsDestroy
destroy: stubs.vsDestroy,
scrollIntoView: stubs.vsScrollIntoView
};

pdfViewer = {
Expand All @@ -42,7 +46,9 @@ describe('ThumbnailsSidebar', () => {
}
};

thumbnailsSidebar = new ThumbnailsSidebar(document.getElementById('test-thumbnails-sidebar'), pdfViewer);
anchorEl = document.getElementById('test-thumbnails-sidebar');

thumbnailsSidebar = new ThumbnailsSidebar(anchorEl, pdfViewer);
});

afterEach(() => {
Expand Down Expand Up @@ -335,6 +341,7 @@ describe('ThumbnailsSidebar', () => {
beforeEach(() => {
stubs.applyCurrentPageSelection = sandbox.stub(thumbnailsSidebar, 'applyCurrentPageSelection');
thumbnailsSidebar.pdfViewer = { pagesCount: 10 };
thumbnailsSidebar.virtualScroller = virtualScroller;
});

const paramaterizedTests = [
Expand All @@ -357,6 +364,7 @@ describe('ThumbnailsSidebar', () => {

expect(thumbnailsSidebar.currentPage).to.be.equal(3);
expect(stubs.applyCurrentPageSelection).to.be.called;
expect(stubs.vsScrollIntoView).to.be.calledWith(2);
});
});

Expand Down Expand Up @@ -403,4 +411,93 @@ describe('ThumbnailsSidebar', () => {
expect(stubs.addClass).to.be.calledOnce;
});
});

describe('toggle()', () => {
beforeEach(() => {
stubs.isOpen = sandbox.stub(thumbnailsSidebar, 'isOpen');
stubs.toggleOpen = sandbox.stub(thumbnailsSidebar, 'toggleOpen');
stubs.toggleClose = sandbox.stub(thumbnailsSidebar, 'toggleClose');
});

it('should do nothing if there is no anchorEl', () => {
thumbnailsSidebar.anchorEl = null;

thumbnailsSidebar.toggle();

expect(stubs.isOpen).not.to.be.called;
expect(stubs.toggleOpen).not.to.be.called;
expect(stubs.toggleClose).not.to.be.called;

thumbnailsSidebar.anchorEl = anchorEl;
});

it('should toggle open if it was closed', () => {
stubs.isOpen.returns(false);

thumbnailsSidebar.toggle();

expect(stubs.isOpen).to.be.called;
expect(stubs.toggleOpen).to.be.called;
expect(stubs.toggleClose).not.to.be.called;
});

it('should toggle closed if it was open', () => {
stubs.isOpen.returns(true);

thumbnailsSidebar.toggle();

expect(stubs.isOpen).to.be.called;
expect(stubs.toggleOpen).not.to.be.called;
expect(stubs.toggleClose).to.be.called;
});
});

describe('toggleOpen()', () => {
beforeEach(() => {
stubs.removeClass = sandbox.stub(thumbnailsSidebar.anchorEl.classList, 'remove');
thumbnailsSidebar.virtualScroller = virtualScroller;
});

it('should do nothing if there is no anchorEl', () => {
thumbnailsSidebar.anchorEl = null;

thumbnailsSidebar.toggleOpen();

expect(stubs.removeClass).not.to.be.called;
expect(stubs.vsScrollIntoView).not.to.be.called;

thumbnailsSidebar.anchorEl = anchorEl;
});

it('should remove the hidden class and scroll the page into view', () => {
thumbnailsSidebar.currentPage = 3;

thumbnailsSidebar.toggleOpen();

expect(stubs.removeClass).to.be.calledWith(CLASS_HIDDEN);
expect(stubs.vsScrollIntoView).to.be.calledWith(2);
});
});

describe('toggleClose()', () => {
beforeEach(() => {
stubs.addClass = sandbox.stub(thumbnailsSidebar.anchorEl.classList, 'add');
});

it('should do nothing if there is no anchorEl', () => {
thumbnailsSidebar.anchorEl = null;

thumbnailsSidebar.toggleClose();

expect(stubs.addClass).not.to.be.called;

thumbnailsSidebar.anchorEl = anchorEl;
});

it('should add the hidden class', () => {
thumbnailsSidebar.toggleClose();

expect(stubs.addClass).to.be.calledWith(CLASS_HIDDEN);
});
});
});
Loading