fix(images): Show UI earlier for large multi-page images #1061
Conversation
|
Verified that @mickr has signed the CLA. Thanks for the pull request! |
| @@ -30,6 +30,7 @@ class ImageBaseViewer extends BaseViewer { | |||
| this.handleMouseUp = this.handleMouseUp.bind(this); | |||
| this.cancelDragEvent = this.cancelDragEvent.bind(this); | |||
| this.finishLoading = this.finishLoading.bind(this); | |||
| this.showUi = this.showUi.bind(this); | |||
|
|
||
| handleFirstImageLoad() { | ||
| if (this.singleImageEls.length > MULTI_PAGE_LOAD_LIMIT) { | ||
| super.setOriginalImageSize(this.imageEl).then(() => { |
There was a problem hiding this comment.
Should this.imageEl be this.singleImageEls[0] instead?
| }); | ||
| } | ||
|
|
||
| this.finishLoading(); |
There was a problem hiding this comment.
Should this be in an else block since if there are more than 10 images we already call showUI?
There was a problem hiding this comment.
No, I don't believe so since there is more work for finishLoading to do with regards to resizing the images. We are just showing the UI earlier in this flow.
| * @return {void} | ||
| */ | ||
| showUi() { | ||
| if (!this.isLoaded()) { |
There was a problem hiding this comment.
Can we make this a guard clause to avoid the nested logic?
| this.imageEl.classList.remove(CLASS_INVISIBLE); | ||
| this.loaded = true; | ||
| this.emit(VIEWER_EVENT.load); | ||
| this.showUi(); |
There was a problem hiding this comment.
Does this need to be a separate method? It doesn't seem like the logical flow has changed and we don't have a showUI method in any other viewer. If so, can we simplify this to:
this.setOriginalImageSize(this.imageEl).then(this.showUI);
| */ | ||
|
|
||
| handleFirstImageLoad() { | ||
| if (this.singleImageEls.length > MULTI_PAGE_LOAD_LIMIT) { |
There was a problem hiding this comment.
Is this limit needed or can we always load the first page as soon as it's ready? Is there a penalty to doing so? If not, it seems like we can remove this entire block and call finishLoading directly.
| * @protected | ||
| * @return {Promise} A promise that is resolved if the original image dimensions were set. | ||
| */ | ||
| setOriginalImageSizes() { |
| /** | ||
| * Handles the load event for the first image. | ||
| * | ||
| * @return {Promise} Promise to load bunch of images |
| @@ -232,7 +232,7 @@ describe('lib/viewers/image/MultiImageViewer', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('setOriginalImageSize()', () => { | |||
| describe('setOriginalImageSizes()', () => { | |||
There was a problem hiding this comment.
Should the child tests now invoke multiImage.setOriginalImageSizes(), instead?
No description provided.