New: Virtual scroll for the thumbnails sidebar #880
Conversation
|
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
src/lib/VirtualScroller.js
Outdated
| this.maxRenderedItems = Math.ceil(this.totalViewItems * 3); | ||
|
|
||
| // Create the scrolling container element | ||
| this.containerEl = document.createElement('div'); |
There was a problem hiding this comment.
Should this be a nav tag?
There was a problem hiding this comment.
In this case, it makes sense to be a nav, but the VirtualScroller is built a bit generically and if used elsewhere nav might not be semantically correct. Thoughts?
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| marginTop: 7.5, | ||
| marginBottom: 7.5, | ||
| renderItemFn: (page) => { | ||
| const thumbnail = document.createElement('button'); |
There was a problem hiding this comment.
We're creating/trashing a lot of objects, but given the low number of living objects at any given time, it should be okay.
There was a problem hiding this comment.
I like this approach for keeping the scroller content agnostic.
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| renderItemFn: (page) => { | ||
| const thumbnail = document.createElement('button'); | ||
| thumbnail.className = 'bp-thumbnail'; | ||
| thumbnail.textContent = `${page}`; |
There was a problem hiding this comment.
Does this work properly in IE11?
Unlike textContent, altering innerText in Internet Explorer (version 11 and below) removes child nodes from the element and permanently destroys all descendant text nodes.
https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent#Differences_from_innerText
There was a problem hiding this comment.
This is just to create the temporary thumbnails for this PR, I'll keep that in mind for when I do a PR to cut the thumbnail images
src/lib/viewers/doc/_docBase.scss
Outdated
| display: flex; | ||
| flex: 1 1 auto; | ||
| justify-content: center; | ||
| margin: 7.5px 15px; |
There was a problem hiding this comment.
Try to avoid fractional pixels when possible.
| flex: 0 0 180px; | ||
| } | ||
|
|
||
| .bp-thumbnail { |
There was a problem hiding this comment.
You may want to look into adding an object-fit rule when images come into play here, depending on what the design looks like.
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| marginTop: 7.5, | ||
| marginBottom: 7.5, | ||
| renderItemFn: (page) => { | ||
| const thumbnail = document.createElement('button'); |
There was a problem hiding this comment.
I like this approach for keeping the scroller content agnostic.
| onScrollHandler(e) { | ||
| const { scrollTop } = e.target; | ||
|
|
||
| if (Math.abs(scrollTop - this.previousScrollTop) > this.maxBufferHeight) { |
There was a problem hiding this comment.
We're doing a decent amount of math here and scroll events fire extremely often. Do you think the frequency of scroll events could cause a perf problem here?
There was a problem hiding this comment.
definitely room for improvement, let me address that in a follow up PR
|
|
||
| this.createListElement = this.createListElement.bind(this); | ||
| this.onScrollHandler = this.onScrollHandler.bind(this); | ||
| this.throttledOnScrollHandler = throttle(this.onScrollHandler, 50); |
There was a problem hiding this comment.
May as well just do this all inline:
this.onScrollHandler = throttle(this.onScrollHandler.bind(this), 50);
| numItemsRendered += 1; | ||
| } | ||
|
|
||
| this.scrollingEl.replaceChild(newListEl, this.listEl); |
|
|
||
| // Get the first page of the document, and use its dimensions | ||
| // to set the thumbnails size of the thumbnails sidebar | ||
| this.pdfViewer.pdfDocument.getPage(1).then((page) => { |
There was a problem hiding this comment.
Consider moving this logic to a ThumbnailSidebar-specific component file.
| this.pdfViewer.pdfDocument.getPage(1).then((page) => { | ||
| const desiredWidth = DEFAULT_THUMBNAILS_SIDEBAR_WIDTH; | ||
| const viewport = page.getViewport(1); | ||
| const scale = desiredWidth / viewport.width; |
There was a problem hiding this comment.
Is it possible for this to return undefined or zero? Can viewport be undefined?
Unit tests to follow in separate PR