Skip to content

Commit

Permalink
Chore: Remove autobind from Document and Presentation Viewers (#507)
Browse files Browse the repository at this point in the history
  • Loading branch information
tonyjin authored Nov 22, 2017
1 parent 32f3c9a commit cd3c67e
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 95 deletions.
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

0 comments on commit cd3c67e

Please sign in to comment.