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.

Sign up for GitHub

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

Jump to bottom

fix(images): Show UI earlier for large multi-page images #1061

Merged
merged 6 commits into from
Aug 29, 2019
Merged

fix(images): Show UI earlier for large multi-page images #1061

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
92f56a4
fix(images): Show UI earlier for large multi-page images
mickr Aug 28, 2019
7794922
fix(images): PR clean up
mickr Aug 29, 2019
9ad59fd
fix(images): Refactored after talking to teammate about structure
mickr Aug 29, 2019
a85ad5a
fix(images): Fix JSDoc
mickr Aug 29, 2019
555cc0e
fix(images): Fix JSDoc and rename test
mickr Aug 29, 2019
e68d615
fix(images): Fix JDoc and update tests
mickr Aug 29, 2019
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
23 changes: 17 additions & 6 deletions src/lib/viewers/image/ImageBaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

showUI?


if (this.isMobile) {
if (Browser.isIOS()) {
Expand Down Expand Up @@ -64,6 +65,21 @@ class ImageBaseViewer extends BaseViewer {
super.destroy();
}

/**
* Shows controls for images and removes loading indicator
*
* @return {void}
*/
showUi() {
if (!this.isLoaded()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this a guard clause to avoid the nested logic?

this.loadUI();
this.zoom();
this.imageEl.classList.remove(CLASS_INVISIBLE);
this.loaded = true;
this.emit(VIEWER_EVENT.load);
}
}

/**
* Finishes loading the images.
*
Expand All @@ -76,12 +92,7 @@ class ImageBaseViewer extends BaseViewer {

const loadOriginalDimensions = this.setOriginalImageSize(this.imageEl);
loadOriginalDimensions.then(() => {
this.loadUI();
this.zoom();

this.imageEl.classList.remove(CLASS_INVISIBLE);
this.loaded = true;
this.emit(VIEWER_EVENT.load);
this.showUi();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);

});
}

Expand Down
20 changes: 19 additions & 1 deletion src/lib/viewers/image/MultiImageViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const PADDING_BUFFER = 100;
const CSS_CLASS_IMAGE = 'bp-images';
const CSS_CLASS_IMAGE_WRAPPER = 'bp-images-wrapper';
const ZOOM_UPDATE_PAN_DELAY = 50;
const MULTI_PAGE_LOAD_LIMIT = 10;

class MultiImageViewer extends ImageBaseViewer {
/** @property {Image[]} - List of images rendered sequentially */
Expand All @@ -23,6 +24,7 @@ class MultiImageViewer extends ImageBaseViewer {
this.handlePageChangeFromScroll = this.handlePageChangeFromScroll.bind(this);
this.handleMultiImageDownloadError = this.handleMultiImageDownloadError.bind(this);
this.handleAssetAndRepLoad = this.handleAssetAndRepLoad.bind(this);
this.handleFirstImageLoad = this.handleFirstImageLoad.bind(this);
}

/**
Expand Down Expand Up @@ -90,6 +92,22 @@ class MultiImageViewer extends ImageBaseViewer {
.catch(this.handleAssetError);
}

/**
* Handles the load event for the first image.
*
* @return {void} Promise to load bunch of images
*/

handleFirstImageLoad() {
if (this.singleImageEls.length > MULTI_PAGE_LOAD_LIMIT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

super.setOriginalImageSize(this.imageEl).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this.imageEl be this.singleImageEls[0] instead?

this.showUi();
});
}

this.finishLoading();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in an else block since if there are more than 10 images we already call showUI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

/**
* Loads the multipart image for viewing
*
Expand Down Expand Up @@ -288,7 +306,7 @@ class MultiImageViewer extends ImageBaseViewer {
*/
bindImageListeners(index) {
if (index === 0) {
this.singleImageEls[index].addEventListener('load', this.finishLoading);
this.singleImageEls[index].addEventListener('load', this.handleFirstImageLoad);
}

this.singleImageEls[index].addEventListener('error', this.handleMultiImageDownloadError);
Expand Down
11 changes: 11 additions & 0 deletions src/lib/viewers/image/__tests__/ImageBaseViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,17 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
imageBase.finishLoading();
expect(stubs.setOriginalImageSize).to.have.been.called;
});

it('should not load UI if previously loaded', () => {
stubs.setOriginalImageSize.returns(Promise.resolve());
imageBase.destroyed = false;
imageBase.loaded = true;

imageBase.finishLoading();
expect(stubs.loadUI).to.not.have.been.called;
expect(stubs.emit).to.not.have.been.called;
expect(stubs.zoom).to.not.have.been.called;
});
});

describe('disableViewerControls()', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/viewers/image/__tests__/MultiImageViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ describe('lib/viewers/image/MultiImageViewer', () => {
.catch(() => {});
});

it('should make the images invisible', () => {
it('should ensure load timer is started', () => {
sandbox.stub(multiImage, 'startLoadTimer');
return multiImage
.load('file/100/content/{page}.png')
Expand Down