From 6080055a6df660c9d7ffeca03dc33373bdbcced1 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 27 Jul 2017 14:04:29 -0700 Subject: [PATCH] Chore: Default single page images to page 1 for annotations (#249) --- .../__tests__/annotatorUtil-test.js | 2 +- src/lib/annotations/annotatorUtil.js | 2 +- src/lib/annotations/doc/DocAnnotator.js | 6 +--- .../doc/__tests__/DocAnnotator-test.js | 33 ++++--------------- src/lib/annotations/image/ImageAnnotator.js | 12 ++++--- src/lib/annotations/image/ImagePointDialog.js | 5 +-- .../image/__tests__/ImagePointDialog-test.js | 20 ++++++----- .../__tests__/ImagePointThread-test.html | 2 +- .../image/__tests__/ImagePointThread-test.js | 33 ++++++++++--------- 9 files changed, 47 insertions(+), 68 deletions(-) diff --git a/src/lib/annotations/__tests__/annotatorUtil-test.js b/src/lib/annotations/__tests__/annotatorUtil-test.js index 86776dfcc..b32369fb7 100644 --- a/src/lib/annotations/__tests__/annotatorUtil-test.js +++ b/src/lib/annotations/__tests__/annotatorUtil-test.js @@ -85,7 +85,7 @@ describe('lib/annotations/annotatorUtil', () => { const barEl = document.querySelector('.bar'); const result = getPageInfo(barEl); assert.equal(result.pageEl, null, 'Page element should be null'); - assert.equal(result.page, -1, 'Page number should be -1'); + assert.equal(result.page, 1, 'Page number should be 1'); }); }); diff --git a/src/lib/annotations/annotatorUtil.js b/src/lib/annotations/annotatorUtil.js index 5070db25a..1ba94184a 100644 --- a/src/lib/annotations/annotatorUtil.js +++ b/src/lib/annotations/annotatorUtil.js @@ -41,7 +41,7 @@ export function findClosestElWithClass(element, className) { */ export function getPageInfo(element) { const pageEl = findClosestElWithClass(element, 'page') || null; - let page = -1; + let page = 1; if (pageEl) { page = parseInt(pageEl.getAttribute('data-page-number'), 10); diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 48b1af506..3dd11cfc4 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -223,11 +223,7 @@ class DocAnnotator extends Annotator { } // Get correct page - let { pageEl, page } = annotatorUtil.getPageInfo(event.target); - if (page === -1) { - // The ( .. ) around assignment is required syntax - ({ pageEl, page } = annotatorUtil.getPageInfo(window.getSelection().anchorNode)); - } + const { pageEl, page } = annotatorUtil.getPageInfo(event.target); // Use highlight module to calculate quad points const { highlightEls } = docAnnotatorUtil.getHighlightAndHighlightEls(this.highlighter, pageEl); diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js index bab1feca7..a0b68e8f6 100644 --- a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js +++ b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js @@ -138,19 +138,10 @@ describe('lib/annotations/doc/DocAnnotator', () => { it('should infer page from selection if it cannot be inferred from event', () => { annotator.highlighter.highlights = [{}, {}]; - stubs.getPageInfo.onFirstCall().returns({ pageEl: null, page: -1 }); - stubs.getPageInfo.onSecondCall().returns({ - pageEl: { - getBoundingClientRect: sandbox.stub().returns({ - width: 100, - height: 100 - }) - }, - page: 2 - }); + stubs.getPageInfo.returns({ pageEl: null, page: -1 }); annotator.getLocationFromEvent({}, TYPES.highlight); - expect(stubs.getPageInfo).to.be.called.twice; + expect(stubs.getPageInfo).to.be.called; }); it('should not return a valid highlight location if no highlights exist', () => { @@ -178,27 +169,15 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); it('should not return a location if there is no selection present', () => { - annotator.highlighter.highlights = []; - const location = annotator.getLocationFromEvent({}, TYPES.highlight_comment); - expect(location).to.be.null; + expect(annotator.getLocationFromEvent({}, TYPES.highlight_comment)).to.be.null; }); it('should infer page from selection if it cannot be inferred from event', () => { - annotator.highlighter.highlights = [{}]; - const getPageStub = stubs.getPageInfo; - getPageStub.onFirstCall().returns({ pageEl: null, page: -1 }); - getPageStub.onSecondCall().returns({ - pageEl: { - getBoundingClientRect: sandbox.stub().returns({ - width: 100, - height: 100 - }) - }, - page: 2 - }); + annotator.highlighter.highlights = [{}, {}]; + stubs.getPageInfo.returns({ pageEl: null, page: -1 }); annotator.getLocationFromEvent({}, TYPES.highlight_comment); - expect(stubs.getSel).to.have.been.called; + expect(stubs.getPageInfo).to.be.called; }); it('should not return a valid highlight location if no highlights exist', () => { diff --git a/src/lib/annotations/image/ImageAnnotator.js b/src/lib/annotations/image/ImageAnnotator.js index 644948ce3..2668b454c 100644 --- a/src/lib/annotations/image/ImageAnnotator.js +++ b/src/lib/annotations/image/ImageAnnotator.js @@ -46,9 +46,6 @@ class ImageAnnotator extends Annotator { // If no image page was selected, ignore, as all images have a page number. const { page } = annotatorUtil.getPageInfo(imageEl); - if (!page) { - return location; - } // Location based only on image position const imageDimensions = imageEl.getBoundingClientRect(); @@ -89,6 +86,13 @@ class ImageAnnotator extends Annotator { */ createAnnotationThread(annotations, location, type) { let thread; + + // Corrects any image annotation page number to 1 instead of -1 + const fixedLocation = location; + if (fixedLocation.page < 0) { + fixedLocation.page = 1; + } + const threadParams = { annotatedElement: this.annotatedElement, annotations, @@ -97,7 +101,7 @@ class ImageAnnotator extends Annotator { fileVersionId: this.fileVersionId, isMobile: this.isMobile, locale: this.locale, - location, + location: fixedLocation, type }; diff --git a/src/lib/annotations/image/ImagePointDialog.js b/src/lib/annotations/image/ImagePointDialog.js index 282fdb8c0..ad97bc98d 100644 --- a/src/lib/annotations/image/ImagePointDialog.js +++ b/src/lib/annotations/image/ImagePointDialog.js @@ -3,7 +3,6 @@ import AnnotationDialog from '../AnnotationDialog'; import * as annotatorUtil from '../annotatorUtil'; import * as imageAnnotatorUtil from './imageAnnotatorUtil'; -const IMAGE_NODE_NAME = 'img'; const PAGE_PADDING_TOP = 15; const POINT_ANNOTATION_ICON_HEIGHT = 31; const POINT_ANNOTATION_ICON_DOT_HEIGHT = 8; @@ -33,9 +32,7 @@ class ImagePointDialog extends AnnotationDialog { const dialogWidth = dialogDimensions.width; // Get image tag inside viewer, based on page number. All images are page 1 by default. - const imageEl = - this.annotatedElement.querySelector(`[data-page-number="${this.location.page || 1}"]`) || - this.annotatedElement.querySelector(IMAGE_NODE_NAME); + const imageEl = this.annotatedElement.querySelector(`[data-page-number="${this.location.page}"]`); // Center middle of dialog with point - this coordinate is with respect to the page let dialogLeftX = browserX - dialogWidth / 2; diff --git a/src/lib/annotations/image/__tests__/ImagePointDialog-test.js b/src/lib/annotations/image/__tests__/ImagePointDialog-test.js index 2fc1cb124..e2d06ebbb 100644 --- a/src/lib/annotations/image/__tests__/ImagePointDialog-test.js +++ b/src/lib/annotations/image/__tests__/ImagePointDialog-test.js @@ -3,7 +3,7 @@ import ImagePointDialog from '../ImagePointDialog'; import * as annotatorUtil from '../../annotatorUtil'; import * as imageAnnotatorUtil from '../imageAnnotatorUtil'; -let pointDialog; +let dialog; const sandbox = sinon.sandbox.create(); describe('lib/annotations/image/ImagePointDialog', () => { @@ -14,21 +14,23 @@ describe('lib/annotations/image/ImagePointDialog', () => { beforeEach(() => { fixture.load('annotations/image/__tests__/ImagePointDialog-test.html'); - pointDialog = new ImagePointDialog({ + dialog = new ImagePointDialog({ annotatedElement: document.querySelector('.annotated-element'), - location: {}, + location: { + page: 1 + }, annotations: [], canAnnotate: true }); - pointDialog.setup([]); - pointDialog.element.style.width = '282px'; + dialog.setup([]); + dialog.element.style.width = '282px'; }); afterEach(() => { sandbox.verifyAndRestore(); - if (typeof pointDialog.destroy === 'function') { - pointDialog.destroy(); - pointDialog = null; + if (typeof dialog.destroy === 'function') { + dialog.destroy(); + dialog = null; } }); @@ -38,7 +40,7 @@ describe('lib/annotations/image/ImagePointDialog', () => { sandbox.stub(annotatorUtil, 'repositionCaret'); sandbox.stub(annotatorUtil, 'showElement'); - pointDialog.position(); + dialog.position(); expect(imageAnnotatorUtil.getBrowserCoordinatesFromLocation).to.have.been.called; expect(annotatorUtil.repositionCaret).to.have.been.called; diff --git a/src/lib/annotations/image/__tests__/ImagePointThread-test.html b/src/lib/annotations/image/__tests__/ImagePointThread-test.html index e3f72d217..26005b1ee 100644 --- a/src/lib/annotations/image/__tests__/ImagePointThread-test.html +++ b/src/lib/annotations/image/__tests__/ImagePointThread-test.html @@ -1,4 +1,4 @@ -
+
diff --git a/src/lib/annotations/image/__tests__/ImagePointThread-test.js b/src/lib/annotations/image/__tests__/ImagePointThread-test.js index 37f03d6e4..c227d467b 100644 --- a/src/lib/annotations/image/__tests__/ImagePointThread-test.js +++ b/src/lib/annotations/image/__tests__/ImagePointThread-test.js @@ -5,7 +5,7 @@ import * as annotatorUtil from '../../annotatorUtil'; import { STATES } from '../../annotationConstants'; import * as imageAnnotatorUtil from '../imageAnnotatorUtil'; -let pointThread; +let thread; const sandbox = sinon.sandbox.create(); describe('lib/annotations/image/ImagePointThread', () => { @@ -16,7 +16,7 @@ describe('lib/annotations/image/ImagePointThread', () => { beforeEach(() => { fixture.load('annotations/image/__tests__/ImagePointThread-test.html'); - pointThread = new ImagePointThread({ + thread = new ImagePointThread({ annotatedElement: document.querySelector('.annotated-element'), annotations: [], annotationService: {}, @@ -35,43 +35,44 @@ describe('lib/annotations/image/ImagePointThread', () => { it('should position and show the thread', () => { sandbox.stub(imageAnnotatorUtil, 'getBrowserCoordinatesFromLocation').returns([1, 2]); sandbox.stub(annotatorUtil, 'showElement'); + sandbox.stub(thread, 'showDialog'); - pointThread.show(); + thread.show(); expect(imageAnnotatorUtil.getBrowserCoordinatesFromLocation).to.have.been.calledWith( - pointThread.location, - pointThread.annotatedElement + thread.location, + thread.annotatedElement ); - expect(annotatorUtil.showElement).to.have.been.calledWith(pointThread.element); + expect(annotatorUtil.showElement).to.have.been.calledWith(thread.element); }); it('should show the dialog if the state is pending', () => { sandbox.stub(imageAnnotatorUtil, 'getBrowserCoordinatesFromLocation').returns([1, 2]); sandbox.stub(annotatorUtil, 'showElement'); - sandbox.stub(pointThread, 'showDialog'); + sandbox.stub(thread, 'showDialog'); - pointThread.state = STATES.pending; - pointThread.show(); + thread.state = STATES.pending; + thread.show(); - expect(pointThread.showDialog).to.have.been.called; + expect(thread.showDialog).to.have.been.called; }); it('should not show the dialog if the state is not pending', () => { sandbox.stub(imageAnnotatorUtil, 'getBrowserCoordinatesFromLocation').returns([1, 2]); sandbox.stub(annotatorUtil, 'showElement'); - sandbox.stub(pointThread, 'showDialog'); + sandbox.stub(thread, 'showDialog'); - pointThread.state = STATES.inactive; - pointThread.show(); + thread.state = STATES.inactive; + thread.show(); - expect(pointThread.showDialog).to.not.have.been.called; + expect(thread.showDialog).to.not.have.been.called; }); }); describe('createDialog', () => { it('should initialize an appropriate dialog', () => { - pointThread.createDialog(); - expect(pointThread.dialog instanceof ImagePointDialog).to.be.true; + thread.createDialog(); + expect(thread.dialog instanceof ImagePointDialog).to.be.true; }); }); });