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: Remove autobind from Document and Presentation Viewers #507

Merged
merged 2 commits into from
Nov 22, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions src/lib/viewers/BaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class BaseViewer extends EventEmitter {
// Bind context for callbacks
this.resetLoadTimeout = this.resetLoadTimeout.bind(this);
this.preventDefault = this.preventDefault.bind(this);
this.debouncedResizeHandler = this.debouncedResizeHandler.bind(this);
this.debouncedResizeHandler = this.getResizeHandler().bind(this);
this.handleAssetError = this.handleAssetError.bind(this);
this.toggleFullscreen = this.toggleFullscreen.bind(this);
this.onFullscreenToggled = this.onFullscreenToggled.bind(this);
Expand Down Expand Up @@ -209,7 +209,7 @@ class BaseViewer extends EventEmitter {
* @private
* @return {Function} debounced resize handler
*/
debouncedResizeHandler() {
getResizeHandler() {
return debounce(() => {
this.resize();
}, RESIZE_WAIT_TIME_IN_MILLIS);
Expand Down
4 changes: 2 additions & 2 deletions src/lib/viewers/__tests__/BaseViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ describe('lib/viewers/BaseViewer', () => {
});
});

describe('debouncedResizeHandler()', () => {
describe('getResizeHandler()', () => {
it('should return a resize handler', () => {
expect(base.debouncedResizeHandler()).to.be.a.function;
expect(base.getResizeHandler()).to.be.a.function;
});
});

Expand Down
82 changes: 51 additions & 31 deletions src/lib/viewers/doc/DocBaseViewer.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import autobind from 'autobind-decorator';
import throttle from 'lodash.throttle';
import BaseViewer from '../BaseViewer';
import Browser from '../../Browser';
Expand Down Expand Up @@ -73,12 +72,31 @@ const LOADING_ICON_MAP = {
xlsx: ICON_FILE_EXCEL
};

@autobind
class DocBaseViewer extends BaseViewer {
//--------------------------------------------------------------------------
// Public
//--------------------------------------------------------------------------

/**
* @inheritdoc
*/
constructor(options) {
super(options);

// Bind context for callbacks
this.handleAssetAndRepLoad = this.handleAssetAndRepLoad.bind(this);
this.print = this.print.bind(this);
this.setPage = this.setPage.bind(this);
this.zoomIn = this.zoomIn.bind(this);
this.zoomOut = this.zoomOut.bind(this);
this.pagerenderedHandler = this.pagerenderedHandler.bind(this);
this.pagechangeHandler = this.pagechangeHandler.bind(this);
this.pagesinitHandler = this.pagesinitHandler.bind(this);
this.enterfullscreenHandler = this.enterfullscreenHandler.bind(this);
this.exitfullscreenHandler = this.exitfullscreenHandler.bind(this);
this.throttledScrollHandler = this.getScrollHandler().bind(this);
}

/**
* @inheritdoc
*/
Expand Down Expand Up @@ -250,21 +268,21 @@ class DocBaseViewer extends BaseViewer {
this.pdfUrl = this.createContentUrlWithAuthParams(template);

return Promise.all([this.loadAssets(JS, CSS), this.getRepStatus().getPromise()])
.then(this.postload)
.then(this.handleAssetAndRepLoad)
.catch(this.handleAssetError);
}

/**
* Loads a document.
* Loads a document after assets and representation are ready.
*
* @return {void}
*/
postload = () => {
handleAssetAndRepLoad() {
this.setupPdfjs();
this.initViewer(this.pdfUrl);
this.initPrint();
this.initFind();
};
}

/**
* Initializes the Find Bar and Find Controller
Expand Down Expand Up @@ -805,7 +823,7 @@ class DocBaseViewer extends BaseViewer {
this.docEl.addEventListener('pagechange', this.pagechangeHandler);

// Detects scroll so an event can be fired
this.docEl.addEventListener('scroll', this.scrollHandler);
this.docEl.addEventListener('scroll', this.throttledScrollHandler);

// Fullscreen
fullscreen.addListener('enter', this.enterfullscreenHandler);
Expand Down Expand Up @@ -834,7 +852,7 @@ class DocBaseViewer extends BaseViewer {
this.docEl.removeEventListener('pagesinit', this.pagesinitHandler);
this.docEl.removeEventListener('pagerendered', this.pagerenderedHandler);
this.docEl.removeEventListener('pagechange', this.pagechangeHandler);
this.docEl.removeEventListener('scroll', this.scrollHandler);
this.docEl.removeEventListener('scroll', this.throttledScrollHandler);

if (this.isMobile) {
if (Browser.isIOS()) {
Expand Down Expand Up @@ -911,7 +929,7 @@ class DocBaseViewer extends BaseViewer {
pageNum: pageNumber
});

// Fire postload event to hide progress bar and cleanup preload after a page is rendered
// Fire progressend event to hide progress bar and cleanup preload after a page is rendered
if (!this.somePageRendered) {
this.hidePreload();
this.emit('progressend');
Expand Down Expand Up @@ -966,34 +984,36 @@ class DocBaseViewer extends BaseViewer {
}

/**
* Scroll handler. Fires an event on start and stop
* Returns throttled handler. Fires an event on start and stop
*
* @private
* @return {void}
*/
scrollHandler = throttle(() => {
// Reset the scroll timer if we are continuing a scroll
if (this.scrollTimer) {
clearTimeout(this.scrollTimer);
}
getScrollHandler() {
return throttle(() => {
// Reset the scroll timer if we are continuing a scroll
if (this.scrollTimer) {
clearTimeout(this.scrollTimer);
}

// only fire the scroll start event if this is a new scroll
if (!this.scrollStarted) {
this.emit('scrollstart', {
scrollTop: this.docEl.scrollTop,
scrollLeft: this.docEl.scrollLeft
});
this.scrollStarted = true;
}
// only fire the scroll start event if this is a new scroll
if (!this.scrollStarted) {
this.emit('scrollstart', {
scrollTop: this.docEl.scrollTop,
scrollLeft: this.docEl.scrollLeft
});
this.scrollStarted = true;
}

this.scrollTimer = setTimeout(() => {
this.emit('scrollend', {
scrollTop: this.docEl.scrollTop,
scrollLeft: this.docEl.scrollLeft
});
this.scrollStarted = false;
}, SCROLL_END_TIMEOUT);
}, SCROLL_EVENT_THROTTLE_INTERVAL);
this.scrollTimer = setTimeout(() => {
this.emit('scrollend', {
scrollTop: this.docEl.scrollTop,
scrollLeft: this.docEl.scrollLeft
});
this.scrollStarted = false;
}, SCROLL_END_TIMEOUT);
}, SCROLL_EVENT_THROTTLE_INTERVAL);
}
}

export default DocBaseViewer;
2 changes: 0 additions & 2 deletions src/lib/viewers/doc/DocumentViewer.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import autobind from 'autobind-decorator';
import DocBaseViewer from './DocBaseViewer';
import DocPreloader from './DocPreloader';
import fullscreen from '../../Fullscreen';
import { ICON_FULLSCREEN_IN, ICON_FULLSCREEN_OUT, ICON_ZOOM_IN, ICON_ZOOM_OUT } from '../../icons/icons';
import './Document.scss';

@autobind
class DocumentViewer extends DocBaseViewer {
//--------------------------------------------------------------------------
// Public
Expand Down
51 changes: 29 additions & 22 deletions src/lib/viewers/doc/PresentationViewer.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import autobind from 'autobind-decorator';
import throttle from 'lodash.throttle';
import DocBaseViewer from './DocBaseViewer';
import PresentationPreloader from './PresentationPreloader';
Expand All @@ -10,12 +9,24 @@ const WHEEL_THROTTLE = 200;
const PADDING_OFFSET = 30;
const SCROLL_EVENT_OFFSET = 5;

@autobind
class PresentationViewer extends DocBaseViewer {
//--------------------------------------------------------------------------
// Public
//--------------------------------------------------------------------------

/**
* @inheritdoc
*/
constructor(options) {
super(options);

// Bind context for callbacks
this.mobileScrollHandler = this.mobileScrollHandler.bind(this);
this.pagesinitHandler = this.pagesinitHandler.bind(this);
this.pagechangeHandler = this.pagechangeHandler.bind(this);
this.throttledWheelHandler = this.getWheelHandler().bind(this);
}

/**
* @inheritdoc
*/
Expand Down Expand Up @@ -143,7 +154,7 @@ class PresentationViewer extends DocBaseViewer {
bindDOMListeners() {
super.bindDOMListeners();

this.docEl.addEventListener('wheel', this.wheelHandler());
this.docEl.addEventListener('wheel', this.throttledWheelHandler);
if (this.hasTouch) {
this.docEl.addEventListener('touchstart', this.mobileScrollHandler);
this.docEl.addEventListener('touchmove', this.mobileScrollHandler);
Expand All @@ -161,7 +172,7 @@ class PresentationViewer extends DocBaseViewer {
unbindDOMListeners() {
super.unbindDOMListeners();

this.docEl.removeEventListener('wheel', this.wheelHandler());
this.docEl.removeEventListener('wheel', this.throttledWheelHandler);
if (this.hasTouch) {
this.docEl.removeEventListener('touchstart', this.mobileScrollHandler);
this.docEl.removeEventListener('touchmove', this.mobileScrollHandler);
Expand Down Expand Up @@ -256,27 +267,23 @@ class PresentationViewer extends DocBaseViewer {
}

/**
* Handles zoom logic around opening the find bar.
* Returns throttled mousewheel handler
*
* @return {void}
* @return {Function} Throttled wheel handler
*/
wheelHandler() {
if (!this.throttledWheelHandler) {
this.throttledWheelHandler = throttle((event) => {
// Should not change pages if there is overflow, horizontal movement or a lack of vertical movement
if (event.deltaY === 0 || event.deltaX !== 0 || this.checkOverflow()) {
return;
}

if (event.deltaY > 0) {
this.nextPage();
} else if (event.deltaY < 0) {
this.previousPage();
}
}, WHEEL_THROTTLE);
}
getWheelHandler() {
return throttle((event) => {
// Should not change pages if there is overflow, horizontal movement or a lack of vertical movement
if (event.deltaY === 0 || event.deltaX !== 0 || this.checkOverflow()) {
return;
}

return this.throttledWheelHandler;
if (event.deltaY > 0) {
this.nextPage();
} else if (event.deltaY < 0) {
this.previousPage();
}
}, WHEEL_THROTTLE);
}

//--------------------------------------------------------------------------
Expand Down
26 changes: 14 additions & 12 deletions src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,20 +347,20 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
sandbox.stub(docBase, 'setup');
Object.defineProperty(BaseViewer.prototype, 'load', { value: sandbox.mock() });
sandbox.stub(docBase, 'createContentUrlWithAuthParams');
sandbox.stub(docBase, 'postload');
sandbox.stub(docBase, 'handleAssetAndRepLoad');
sandbox.stub(docBase, 'getRepStatus').returns({ getPromise: () => Promise.resolve() });
sandbox.stub(docBase, 'loadAssets');

return docBase.load().then(() => {
expect(docBase.loadAssets).to.be.called;
expect(docBase.setup).to.be.called;
expect(docBase.createContentUrlWithAuthParams).to.be.calledWith('foo');
expect(docBase.postload).to.be.called;
expect(docBase.handleAssetAndRepLoad).to.be.called;
});
});
});

describe('postload', () => {
describe('handleAssetAndRepLoad', () => {
it('should setup pdfjs, init viewer, print, and find', () => {
const url = 'foo';
docBase.pdfUrl = url;
Expand All @@ -373,7 +373,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
const initPrintStub = sandbox.stub(docBase, 'initPrint');
const initFindStub = sandbox.stub(docBase, 'initFind');

docBase.postload();
docBase.handleAssetAndRepLoad();

expect(setupPdfjsStub).to.be.called;
expect(initViewerStub).to.be.calledWith(docBase.pdfUrl);
Expand Down Expand Up @@ -1168,7 +1168,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
expect(stubs.addEventListener).to.be.calledWith('pagesinit', docBase.pagesinitHandler);
expect(stubs.addEventListener).to.be.calledWith('pagerendered', docBase.pagerenderedHandler);
expect(stubs.addEventListener).to.be.calledWith('pagechange', docBase.pagechangeHandler);
expect(stubs.addEventListener).to.be.calledWith('scroll', docBase.scrollHandler);
expect(stubs.addEventListener).to.be.calledWith('scroll', docBase.throttledScrollHandler);

expect(stubs.addEventListener).to.not.be.calledWith('gesturestart', docBase.mobileZoomStartHandler);
expect(stubs.addEventListener).to.not.be.calledWith('gestureend', docBase.mobileZoomEndHandler);
Expand Down Expand Up @@ -1209,7 +1209,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
expect(stubs.removeEventListener).to.be.calledWith('pagesinit', docBase.pagesinitHandler);
expect(stubs.removeEventListener).to.be.calledWith('pagerendered', docBase.pagerenderedHandler);
expect(stubs.removeEventListener).to.be.calledWith('pagechange', docBase.pagechangeHandler);
expect(stubs.removeEventListener).to.be.calledWith('scroll', docBase.scrollHandler);
expect(stubs.removeEventListener).to.be.calledWith('scroll', docBase.throttledScrollHandler);
});

it('should not remove the doc element listeners if the doc element does not exist', () => {
Expand Down Expand Up @@ -1306,7 +1306,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
expect(stubs.emit).to.be.calledWith('scale', { pageNum: 1, scale: 0.5 });
});

it('should emit postload event if not already emitted', () => {
it('should emit handleAssetAndRepLoad event if not already emitted', () => {
docBase.pagerenderedHandler(docBase.event);
expect(stubs.emit).to.be.calledWith('progressend');
});
Expand Down Expand Up @@ -1384,28 +1384,30 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
});
});

describe('scrollHandler()', () => {
describe('getScrollHandler()', () => {
let scrollHandler;

beforeEach(() => {
stubs.emit = sandbox.stub(docBase, 'emit');
docBase.scrollStarted = false;
scrollHandler = docBase.getScrollHandler();
});

it('should emit the scrollstart event on a new scroll', () => {
docBase.scrollHandler();
scrollHandler();
expect(stubs.emit).to.be.calledWith('scrollstart');
});

it('should not emit the scrollstart event on a continued scroll', () => {
docBase.scrollStarted = true;

docBase.scrollHandler();
scrollHandler();
expect(stubs.emit).to.not.be.calledWith('scrollstart');
});

it('should emit a scrollend event after scroll timeout', () => {
const clock = sinon.useFakeTimers();

docBase.scrollHandler();
scrollHandler();
expect(stubs.emit).to.be.calledWith('scrollstart');

clock.tick(SCROLL_END_TIMEOUT + 1);
Expand Down
Loading