Update: Handle non uniform thumbnail sizes #896
Conversation
ConradJChan
commented
Jan 18, 2019
ConradJChan
commented
|
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
src/lib/ThumbnailsSidebar.js
Outdated
| // In case the current page ratio is same as the first page | ||
| // or in case it's larger (which means that it's wider), keep | ||
| // the width at the max thumbnail width | ||
| canvasWidth = THUMBNAIL_WIDTH_MAX; |
There was a problem hiding this comment.
Could this live up with let canvasWidth = THUMBNAIL_WIDTH_MAX as a default value?
There was a problem hiding this comment.
will get rid of canvasWidth
| const canvas = document.createElement('canvas'); | ||
|
|
||
| return this.pdfViewer.pdfDocument | ||
| .getPage(itemIndex + 1) | ||
| .getPage(pageNum) | ||
| .then((page) => { |
There was a problem hiding this comment.
Is it worth moving these to named class methods to avoid recreating the functions each time? It usually wouldn't matter, but I think we're doing this many times a second on scroll, right?
There was a problem hiding this comment.
This should only get invoked at when the debounced scroll handler fires, and when the image isn't already cached, so maybe it's ok as is?
| // Add the height and width to the image to be the same as the thumbnail | ||
| // so that the css `object-fit` rule will work | ||
| imageEl.style.width = `${DEFAULT_THUMBNAILS_SIDEBAR_WIDTH}px`; | ||
| imageEl.style.height = `${DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / this.pageRatio}px`; |
There was a problem hiding this comment.
Any rounding required to avoid fractional pixels?
There was a problem hiding this comment.
It seems like the modern desktop browsers all support sub pixel rendering right now, i'll investigate and maybe do a follow up PR to see if I can get away from fractional values
src/lib/viewers/doc/_docBase.scss
Outdated
| @@ -36,9 +36,8 @@ $thumbnail-border-radius: 4px; | |||
| } | |||
|
|
|||
| .bp-thumbnail-image { | |||
| border-radius: $thumbnail-border-radius; | |||
| object-fit: contain; | |||
There was a problem hiding this comment.
What's the experience like in IE11? https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit#Browser_compatibility
There was a problem hiding this comment.
updating PR to use background-image
src/lib/ThumbnailsSidebar.js
Outdated
| @@ -49,6 +49,8 @@ class ThumbnailsSidebar { | |||
| this.createThumbnailImage = this.createThumbnailImage.bind(this); | |||
| this.generateThumbnailImages = this.generateThumbnailImages.bind(this); | |||
| this.thumbnailClickHandler = this.thumbnailClickHandler.bind(this); | |||
| this.getThumbnailDataURL = this.getThumbnailDataURL.bind(this); | |||
| this.createImageEl = this.createImageEl.bind(this); | |||
There was a problem hiding this comment.
How much would I need to bribe you to alphabetize these?
