Skip to content

Commit

Permalink
Chore: refactored and renamed getPageElAndPageNumber (#162)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JustinHoldstock
JustinHoldstock authored Jun 6, 2017
1 parent 4d31542 commit 00dedab
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 37 deletions.
8 changes: 4 additions & 4 deletions src/lib/annotations/__tests__/annotatorUtil-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import {
findClosestElWithClass,
findClosestDataType,
getPageElAndPageNumber,
getPageInfo,
showElement,
hideElement,
showInvisibleElement,
Expand Down Expand Up @@ -66,18 +66,18 @@ describe('lib/annotations/annotatorUtil', () => {
});
});

describe('getPageElAndPageNumber()', () => {
describe('getPageInfo()', () => {
it('should return page element and page number that the specified element is on', () => {
const fooEl = document.querySelector('.foo');
const pageEl = document.querySelector('.page');
const result = getPageElAndPageNumber(fooEl);
const result = getPageInfo(fooEl);
assert.equal(result.pageEl, pageEl, 'Page element should be equal');
assert.equal(result.page, 2, 'Page number should be equal');
});

it('should return no page element and -1 page number if no page is found', () => {
const barEl = document.querySelector('.bar');
const result = getPageElAndPageNumber(barEl);
const result = getPageInfo(barEl);
assert.equal(result.pageEl, null, 'Page element should be null');
assert.equal(result.page, -1, 'Page number should be -1');
});
Expand Down
16 changes: 6 additions & 10 deletions src/lib/annotations/annotatorUtil.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,15 @@ export function findClosestElWithClass(element, className) {
* @param {HTMLElement} element - Element to find page and page number for
* @return {Object} Page element/page number if found or null/-1 if not
*/
export function getPageElAndPageNumber(element) {
const pageEl = findClosestElWithClass(element, 'page');
export function getPageInfo(element) {
const pageEl = findClosestElWithClass(element, 'page') || null;
let page = -1;

if (pageEl) {
return {
pageEl,
page: parseInt(pageEl.getAttribute('data-page-number'), 10)
};
page = parseInt(pageEl.getAttribute('data-page-number'), 10);
}

return {
pageEl: null,
page: -1
};
return { pageEl, page };
}

/**
Expand Down
15 changes: 7 additions & 8 deletions src/lib/annotations/doc/DocAnnotator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ function isThreadInHoverState(thread) {

// If click isn't on a page, ignore
const eventTarget = event.target;
const { pageEl, page } = annotatorUtil.getPageElAndPageNumber(eventTarget);
const { pageEl, page } = annotatorUtil.getPageInfo(eventTarget);
if (!pageEl) {
return location;
}
Expand Down Expand Up @@ -153,10 +153,10 @@ function isThreadInHoverState(thread) {
}

// Get correct page
let { pageEl, page } = annotatorUtil.getPageElAndPageNumber(event.target);
let { pageEl, page } = annotatorUtil.getPageInfo(event.target);
if (page === -1) {
// The ( .. ) around assignment is required syntax
({ pageEl, page } = annotatorUtil.getPageElAndPageNumber(window.getSelection().anchorNode));
({ pageEl, page } = annotatorUtil.getPageInfo(window.getSelection().anchorNode));
}

// Use Rangy to save the current selection because using the
Expand Down Expand Up @@ -199,8 +199,7 @@ function isThreadInHoverState(thread) {
}

/**
* Creates the proper type of thread, adds it to in-memory map, and returns
* it.
* Creates the proper type of thread, adds it to in-memory map, and returns it.
*
* @override
* @param {Annotation[]} annotations - Annotations in thread
Expand Down Expand Up @@ -444,7 +443,7 @@ function isThreadInHoverState(thread) {
this.mouseMoveEvent = null;
this.throttleTimer = performance.now();
// Only filter through highlight threads on the current page
const { page } = annotatorUtil.getPageElAndPageNumber(event.target);
const { page } = annotatorUtil.getPageInfo(event.target);
const pageThreads = this.getHighlightThreadsOnPage(page);
const delayThreads = [];
let hoverActive = false;
Expand Down Expand Up @@ -557,7 +556,7 @@ function isThreadInHoverState(thread) {

// Only filter through highlight threads on the current page
// Reset active highlight threads before creating new highlight
const page = annotatorUtil.getPageElAndPageNumber(event.target).page;
const page = annotatorUtil.getPageInfo(event.target).page;
const activeThreads = this.getHighlightThreadsOnPage(page).filter(
(thread) => constants.ACTIVE_STATES.indexOf(thread.state) > -1
);
Expand Down Expand Up @@ -604,7 +603,7 @@ function isThreadInHoverState(thread) {
});

// Only filter through highlight threads on the current page
const page = annotatorUtil.getPageElAndPageNumber(event.target).page;
const page = annotatorUtil.getPageInfo(event.target).page;
const pageThreads = this.getHighlightThreadsOnPage(page);
pageThreads.forEach((thread) => {
// We use this to prevent a mousedown from activating two different
Expand Down
28 changes: 14 additions & 14 deletions src/lib/annotations/doc/__tests__/DocAnnotator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
annotator.annotatedElement = annotator.getAnnotatedEl(document);
annotator.annotationService = {};

stubs.getPage = sandbox.stub(annotatorUtil, 'getPageElAndPageNumber');
stubs.getPageInfo = sandbox.stub(annotatorUtil, 'getPageInfo');
});

afterEach(() => {
Expand Down Expand Up @@ -93,13 +93,13 @@ describe('lib/annotations/doc/DocAnnotator', () => {

it('should not return a location if click isn\'t on page', () => {
stubs.selection.returns(false);
stubs.getPage.returns({ pageEl: null, page: -1 });
stubs.getPageInfo.returns({ pageEl: null, page: -1 });
expect(annotator.getLocationFromEvent({}, 'point')).to.be.null;
});

it('should not return a location if click is on dialog', () => {
stubs.selection.returns(false);
stubs.getPage.returns({
stubs.getPageInfo.returns({
pageEl: document.querySelector('.annotated-element'),
page: 1
});
Expand All @@ -110,7 +110,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
page = 2;

stubs.selection.returns(false);
stubs.getPage.returns({ pageEl: stubs.pageEl, page });
stubs.getPageInfo.returns({ pageEl: stubs.pageEl, page });
stubs.findClosest.returns('not-a-dialog');
sandbox.stub(docAnnotatorUtil, 'convertDOMSpaceToPDFSpace').returns([x, y]);

Expand All @@ -126,8 +126,8 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should infer page from selection if it cannot be inferred from event', () => {
stubs.getPage.onFirstCall().returns({ pageEl: null, page: -1 });
stubs.getPage.onSecondCall().returns({
stubs.getPageInfo.onFirstCall().returns({ pageEl: null, page: -1 });
stubs.getPageInfo.onSecondCall().returns({
pageEl: {
getBoundingClientRect: sandbox.stub().returns({
width: 100,
Expand All @@ -142,12 +142,12 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should not return a valid highlight location if no highlights exist', () => {
stubs.getPage.returns({ pageEl: stubs.pageEl, page });
stubs.getPageInfo.returns({ pageEl: stubs.pageEl, page });
expect(annotator.getLocationFromEvent({}, 'highlight')).to.deep.equal(null);
});

it('should return a valid highlight location if selection is valid', () => {
stubs.getPage.returns({ pageEl: stubs.pageEl, page });
stubs.getPageInfo.returns({ pageEl: stubs.pageEl, page });
stubs.points.onFirstCall().returns(quadPoints[0]);
stubs.points.onSecondCall().returns(quadPoints[1]);

Expand All @@ -166,7 +166,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should infer page from selection if it cannot be inferred from event', () => {
const getPageStub = stubs.getPage;
const getPageStub = stubs.getPageInfo;
getPageStub.onFirstCall().returns({ pageEl: null, page: -1 });
getPageStub.onSecondCall().returns({
pageEl: {
Expand All @@ -183,12 +183,12 @@ describe('lib/annotations/doc/DocAnnotator', () => {
});

it('should not return a valid highlight location if no highlights exist', () => {
stubs.getPage.returns({ pageEl: stubs.pageEl, page });
stubs.getPageInfo.returns({ pageEl: stubs.pageEl, page });
expect(annotator.getLocationFromEvent({}, 'highlight-comment')).to.deep.equal(null);
});

it('should return a valid highlight location if selection is valid', () => {
stubs.getPage.returns({ pageEl: stubs.pageEl, page });
stubs.getPageInfo.returns({ pageEl: stubs.pageEl, page });
stubs.points.onFirstCall().returns(quadPoints[0]);
stubs.points.onSecondCall().returns(quadPoints[1]);
stubs.getHighlights.returns({ highlight: {}, highlightEls: [{}, {}] });
Expand Down Expand Up @@ -511,7 +511,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
};
stubs.delayMock = sandbox.mock(stubs.delayThread);

stubs.getPage = stubs.getPage.returns({ pageEl: {}, page: 1 });
stubs.getPageInfo = stubs.getPageInfo.returns({ pageEl: {}, page: 1 });
stubs.getThreads = sandbox.stub(annotator, 'getHighlightThreadsOnPage');
stubs.clock = sinon.useFakeTimers();

Expand Down Expand Up @@ -634,7 +634,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
};
stubs.threadMock = sandbox.mock(stubs.thread);

stubs.getPage = stubs.getPage.returns({ pageEl: {}, page: 1 });
stubs.getPageInfo = stubs.getPageInfo.returns({ pageEl: {}, page: 1 });
stubs.hasActiveDialog = sandbox.stub(docAnnotatorUtil, 'hasActiveDialog').returns(false);
stubs.getThreads = sandbox.stub(annotator, 'getHighlightThreadsOnPage').returns([]);
stubs.getLocation = sandbox.stub(annotator, 'getLocationFromEvent').returns(undefined);
Expand Down Expand Up @@ -707,7 +707,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
};
stubs.threadMock = sandbox.mock(stubs.thread);

stubs.getPage = stubs.getPage.returns({ pageEl: {}, page: 1 });
stubs.getPageInfo = stubs.getPageInfo.returns({ pageEl: {}, page: 1 });
stubs.getAllThreads = sandbox.stub(annotator, 'getThreadsWithStates').returns([]);
stubs.getThreads = sandbox.stub(annotator, 'getHighlightThreadsOnPage').returns([stubs.thread]);
});
Expand Down
2 changes: 1 addition & 1 deletion src/lib/annotations/image/ImageAnnotator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const ANNOTATED_ELEMENT_SELECTOR = '.bp-image, .bp-images-wrapper';
}

// If no image page was selected, ignore, as all images have a page number.
const { page } = annotatorUtil.getPageElAndPageNumber(imageEl);
const { page } = annotatorUtil.getPageInfo(imageEl);
if (!page) {
return location;
}
Expand Down

0 comments on commit 00dedab

Please sign in to comment.