Update: Thumbnails sidebar images #887
Conversation
| @@ -134,6 +153,7 @@ class VirtualScroller { | |||
| */ | |||
| bindDOMListeners() { | |||
| this.scrollingEl.addEventListener('scroll', this.throttledOnScrollHandler, { passive: true }); | |||
There was a problem hiding this comment.
These are both bound to the scroll event but one is an end handler and the other is just a handler. scroll fires on scroll end?
There was a problem hiding this comment.
Yeah one is a throttled scroll handler which will be used to render the placeholder thumbnails. The debounced scroll handler is for when scrolling has finished so we can determine which thumbnail images to render
| // and what needs to be rendered | ||
| const { startOffset: curStartOffset, endOffset: curEndOffset } = this.getInfo(); | ||
|
|
||
| if (curStartOffset === offset) { |
There was a problem hiding this comment.
What if we haven't hit our maxRenderedItems? Even if we're starting from the same place could we want to render additional stuff? This might not be an issue now, but if we later add resizing maybe more thumbnails could be visible after a resize but we're still starting from the same spot?
There was a problem hiding this comment.
well right now we always try to render maxRenderedItems unless it surpasses the totalItems so this seems to be ok. I'll keep an eye on it when we get to the resize stuff
| margin: THUMBNAIL_MARGIN, | ||
| renderItemFn: this.createPlaceholderThumbnail, | ||
| onScrollEnd: this.generateThumbnailImages, | ||
| onInit: this.generateThumbnailImages |
There was a problem hiding this comment.
Minor, but the name makes it seem like this should be passed into the constructor.
src/lib/VirtualScroller.js
Outdated
| * The newList element is modified. | ||
| * | ||
| * @param {HTMLElement} newList - the new `li` element | ||
| * @param {Array<HTMLElement>} oldList - the children of the old `li` element |
There was a problem hiding this comment.
Minor, but it seems inconsistent to have newList represent a parent ol, but oldList is the children of the old ol. It also looks like the comments may need to be corrected from li -> ol.
There was a problem hiding this comment.
Made this consistent now
|
I'll add unit tests in a follow up PR, I just made the existing tests pass for now |
Now cuts thumbnail images to insert into sidebar. Current approach as to *when* to render thumbnail images is to introduce a `onScrollEnd` callback from the `VirtualScroller` so that when scrolling ends, we look at what list items are rendered and then ask PDFJS to generate the thumbnail image. The `VirtualScroller` `renderItemFn` is invoked by a throttled scroll event handler which is used to create a placeholder thumbnail which includes the page number indicator
Now cuts thumbnail images to insert into sidebar. Current approach as to *when* to render thumbnail images is to introduce a `onScrollEnd` callback from the `VirtualScroller` so that when scrolling ends, we look at what list items are rendered and then ask PDFJS to generate the thumbnail image. The `VirtualScroller` `renderItemFn` is invoked by a throttled scroll event handler which is used to create a placeholder thumbnail which includes the page number indicator
Now cuts thumbnail images to insert into sidebar. Current approach as to *when* to render thumbnail images is to introduce a `onScrollEnd` callback from the `VirtualScroller` so that when scrolling ends, we look at what list items are rendered and then ask PDFJS to generate the thumbnail image. The `VirtualScroller` `renderItemFn` is invoked by a throttled scroll event handler which is used to create a placeholder thumbnail which includes the page number indicator
Tests to follow