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.

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

chore(loading): Remove sub-header progress bar experience #1348

Merged
merged 1 commit into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 1 addition & 16 deletions src/lib/Preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -870,10 +870,9 @@ class Preview extends EventEmitter {
// Update navigation
this.ui.showNavigation(this.file.id, this.collection);

// Setup loading UI and progress bar
// Setup loading indicator
this.ui.showLoadingIcon(this.file.extension);
this.ui.showLoadingIndicator();
this.ui.startProgressBar();

// Start the preview duration timer when the user starts to perceive preview's load
const previewDurationTag = Timer.createTag(this.file.id, DURATION_METRIC);
Expand Down Expand Up @@ -929,9 +928,6 @@ class Preview extends EventEmitter {
// Whether the loading indicator should be shown
this.options.showLoading = options.showLoading !== false;

// Whether the progress indicator should be shown
this.options.showProgress = options.showProgress !== false;

// Whether annotations v4 buttons should be shown in toolbar
this.options.showAnnotationsControls = !!options.showAnnotationsControls;

Expand Down Expand Up @@ -1264,12 +1260,6 @@ class Preview extends EventEmitter {
case VIEWER_EVENT.load:
this.finishLoading(data.data);
break;
case VIEWER_EVENT.progressStart:
this.ui.startProgressBar();
break;
case VIEWER_EVENT.progressEnd:
this.ui.finishProgressBar();
break;
case VIEWER_EVENT.error:
// Do nothing since 'error' event was already caught, and will be emitted
// as a 'preview_error' event
Expand Down Expand Up @@ -1373,11 +1363,6 @@ class Preview extends EventEmitter {
}
}

// Finish the progress bar unless instructed not to
if (data.endProgress !== false) {
this.ui.finishProgressBar();
}

// Programmatically focus on the viewer after it loads
if (this.options.autoFocus && this.viewer && this.viewer.containerEl) {
this.viewer.containerEl.focus();
Expand Down
1 change: 0 additions & 1 deletion src/lib/Preview.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@
@import 'loading';
@import 'navigation';
@import './Controls';
@import './ProgressBar';
@import './VirtualScroller';
37 changes: 0 additions & 37 deletions src/lib/PreviewUI.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import LoadingIcon from './LoadingIcon';
import Notification from './Notification';
import ProgressBar from './ProgressBar';
import shellTemplate from './shell.html';
import {
CLASS_BOX_PREVIEW_BASE_HEADER,
Expand Down Expand Up @@ -49,20 +48,13 @@ class PreviewUI {
/** @property {HTMLElement} - Preview container element which houses the sidebar and content */
previewContainer;

/** @property {ProgressBar} - Progress bar instance */
progressBar;

/**
* Destroy preview container content.
*
* @public
* @return {void}
*/
cleanup() {
if (this.progressBar) {
this.progressBar.destroy();
}

if (this.previewContainer) {
this.previewContainer.removeEventListener('mousemove', this.mousemoveHandler);
}
Expand Down Expand Up @@ -131,11 +123,6 @@ class PreviewUI {
this.destroyLoading();
}

// Setup progress bar
if (options.showProgress) {
this.progressBar = new ProgressBar(this.container);
}

// Attach keyboard events
document.addEventListener('keydown', this.keydownHandler);

Expand Down Expand Up @@ -274,30 +261,6 @@ class PreviewUI {
this.previewContainer.classList.add(CLASS_PREVIEW_LOADED);
}

/**
* Shows and starts a progress bar at the top of the preview.
*
* @public
* @return {void}
*/
startProgressBar() {
if (this.progressBar) {
this.progressBar.start();
}
}

/**
* Finishes and hides the top progress bar if present.
*
* @public
* @return {void}
*/
finishProgressBar() {
if (this.progressBar) {
this.progressBar.finish();
}
}

/**
* Setup notification
*
Expand Down
127 changes: 0 additions & 127 deletions src/lib/ProgressBar.js

This file was deleted.

22 changes: 0 additions & 22 deletions src/lib/ProgressBar.scss

This file was deleted.

38 changes: 0 additions & 38 deletions src/lib/__tests__/Preview-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1218,7 +1218,6 @@ describe('lib/Preview', () => {
previewUIMock.expects('setup');
previewUIMock.expects('showLoadingIcon');
previewUIMock.expects('showLoadingIndicator');
previewUIMock.expects('startProgressBar');
previewUIMock.expects('showNavigation');
previewUIMock.expects('setupNotification');

Expand Down Expand Up @@ -1320,15 +1319,6 @@ describe('lib/Preview', () => {
expect(preview.options.showLoading).toBe(false);
});

test('should set whether to show progress indicators', () => {
preview.parseOptions(preview.previewOptions);
expect(preview.options.showProgress).toBe(true);

preview.previewOptions.showProgress = false;
preview.parseOptions(preview.previewOptions);
expect(preview.options.showProgress).toBe(false);
});

test('should set whether to skip load from the server and any server updates', () => {
preview.parseOptions(preview.previewOptions);
expect(preview.options.skipServerUpdate).toBe(false);
Expand Down Expand Up @@ -1784,11 +1774,6 @@ describe('lib/Preview', () => {
});

describe('handleViewerEvents()', () => {
beforeEach(() => {
jest.spyOn(preview.ui, 'startProgressBar').mockImplementation();
jest.spyOn(preview.ui, 'finishProgressBar').mockImplementation();
});

test('should call download on download event', () => {
jest.spyOn(preview, 'download').mockImplementation();
preview.handleViewerEvents({ event: VIEWER_EVENT.download });
Expand All @@ -1807,16 +1792,6 @@ describe('lib/Preview', () => {
expect(preview.finishLoading).toBeCalled();
});

test('should start progress bar on progressstart event', () => {
preview.handleViewerEvents({ event: VIEWER_EVENT.progressStart });
expect(preview.ui.startProgressBar).toBeCalled();
});

test('should finish progress bar on progressend event', () => {
preview.handleViewerEvents({ event: VIEWER_EVENT.progressEnd });
expect(preview.ui.finishProgressBar).toBeCalled();
});

test('should emit viewerevent when event does not match', () => {
jest.spyOn(preview, 'emit');
const data = {
Expand Down Expand Up @@ -1867,7 +1842,6 @@ describe('lib/Preview', () => {
stubs.emit = jest.spyOn(preview, 'emit');
stubs.logPreviewEvent = jest.spyOn(preview, 'logPreviewEvent');
stubs.prefetchNextFiles = jest.spyOn(preview, 'prefetchNextFiles');
stubs.finishProgressBar = jest.spyOn(preview.ui, 'finishProgressBar').mockImplementation();
stubs.setupNotification = jest.spyOn(preview.ui, 'setupNotification').mockImplementation();

stubs.perf = {
Expand Down Expand Up @@ -1970,18 +1944,6 @@ describe('lib/Preview', () => {
expect(callPhantomSpy).toBeCalled();
});

test('should postload if skipPostload is not true', () => {
preview.finishLoading();
expect(stubs.finishProgressBar).toBeCalled();
});

test('should skip postload if skipPostload is true', () => {
preview.finishLoading({
endProgress: false,
});
expect(stubs.finishProgressBar).not.toBeCalled();
});

test('should focus the viewer container', () => {
preview.options.autoFocus = true;
preview.viewer.containerEl = {
Expand Down
Loading