Skip to content

Commit

Permalink
Fix: Reset popover UI on re-render/scale events (#284)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pramodsum
pramodsum committed Nov 16, 2018
1 parent 12d5f6d commit 43e5cb0
Show file tree
Hide file tree
Showing 9 changed files with 123 additions and 60 deletions.
12 changes: 6 additions & 6 deletions src/AnnotationThread.js
View file
Expand Up @@ -209,15 +209,15 @@ class AnnotationThread extends EventEmitter {
*/
unmountPopover() {
this.reset();

this.toggleFlippedThreadEl();

const pageEl = this.getPopoverParent();
const popoverLayer = pageEl.querySelector('.ba-dialog-layer');
if (this.popoverComponent && popoverLayer) {
unmountComponentAtNode(popoverLayer);
this.popoverComponent = null;
const popoverLayers = this.container.querySelectorAll('.ba-dialog-layer');
if (!this.popoverComponent || popoverLayers.length === 0) {
return;
}

popoverLayers.forEach(unmountComponentAtNode);
this.popoverComponent = null;
}

/**
Expand Down
22 changes: 14 additions & 8 deletions src/Annotator.js
View file
Expand Up @@ -180,19 +180,17 @@ class Annotator extends EventEmitter {
* @param {AnnotationType} annotationType - Type of annotation
* @return {Object} Location object
*/
/* eslint-disable no-unused-vars */
/* eslint-disable-next-line no-unused-vars */
getLocationFromEvent = (event: Event, annotationType: AnnotationType): ?Location => {};
/* eslint-enable no-unused-vars */

/**
* Must be implemented to determine the annotated element in the viewer.
*
* @param {HTMLElement} containerEl - Container element for the viewer
* @return {HTMLElement} Annotated element in the viewer
*/
/* eslint-disable no-unused-vars */
/* eslint-disable-next-line no-unused-vars */
getAnnotatedEl(containerEl: HTMLElement): ?HTMLElement {}
/* eslint-enable no-unused-vars */

/**
* Annotations setup.
Expand Down Expand Up @@ -397,16 +395,23 @@ class Annotator extends EventEmitter {
return thread;
}
/**
* Resets any annotation UI on render/scale events
*
* @param {number} [pageNum] - optional page number
* @return {void}
*/
/* eslint-disable-next-line no-unused-vars */
resetAnnotationUI(pageNum?: number) {}
/**
* Renders annotations from memory.
*
* @return {void}
*/
render() {
Object.keys(this.modeControllers).forEach((mode) => {
const controller = this.modeControllers[mode];
controller.render();
});
this.resetAnnotationUI();
Object.keys(this.modeControllers).forEach((mode) => this.modeControllers[mode].render());
}
/**
Expand All @@ -416,6 +421,7 @@ class Annotator extends EventEmitter {
* @return {void}
*/
renderPage(pageNum: number) {
this.resetAnnotationUI(pageNum);
Object.keys(this.modeControllers).forEach((mode) => this.modeControllers[mode].renderPage(pageNum));
}
Expand Down
28 changes: 28 additions & 0 deletions src/__tests__/AnnotationThread-test.js
View file
Expand Up @@ -61,6 +61,34 @@ describe('AnnotationThread', () => {
});
});

describe('unmountPopover', () => {
beforeEach(() => {
thread.popoverComponent = {};
thread.reset = jest.fn();
thread.toggleFlippedThreadEl = jest.fn();
thread.container = {
querySelectorAll: jest.fn().mockReturnValue([rootElement])
};
});

it('should there are no popover layers', () => {
thread.container = {
querySelectorAll: jest.fn().mockReturnValue([])
};
thread.unmountPopover();
expect(thread.popoverComponent).not.toBeNull();
expect(thread.reset).toBeCalled();
expect(thread.toggleFlippedThreadEl).toBeCalled();
});

it('should unmount any visible popovers', () => {
thread.unmountPopover();
expect(thread.popoverComponent).toBeNull();
expect(thread.reset).toBeCalled();
expect(thread.toggleFlippedThreadEl).toBeCalled();
});
});

describe('hide()', () => {
it('should hide the thread element', () => {
thread.unmountPopover = jest.fn();
Expand Down
3 changes: 1 addition & 2 deletions src/controllers/AnnotationModeController.js
View file
Expand Up @@ -329,9 +329,8 @@ class AnnotationModeController extends EventEmitter {
* @param {Object} params - Annotation thread params
* @return {AnnotationThread|null} Annotation thread instance or null
*/
/* eslint-disable no-unused-vars */
/* eslint-disable-next-line no-unused-vars */
instantiateThread(params: Object): AnnotationThread {
/* eslint-enable no-unused-vars */
throw new Error('Implement me!');
}

Expand Down
14 changes: 10 additions & 4 deletions src/doc/CreateHighlightDialog.js
View file
Expand Up @@ -79,12 +79,18 @@ class CreateHighlightDialog extends EventEmitter {
* @return {void}
*/
unmountPopover() {
if (!this.isVisible) {
return;
}

this.isVisible = false;
const popoverLayer = this.container.querySelector('.ba-dialog-layer');
if (this.createPopoverComponent && popoverLayer) {
unmountComponentAtNode(popoverLayer);
this.createPopoverComponent = null;
const popoverLayers = this.container.querySelectorAll('.ba-dialog-layer');
if (!this.createPopoverComponent || popoverLayers.length === 0) {
return;
}

popoverLayers.forEach(unmountComponentAtNode);
this.createPopoverComponent = null;
}

/**
Expand Down
22 changes: 11 additions & 11 deletions src/doc/DocAnnotator.js
View file
Expand Up @@ -76,6 +76,8 @@ class DocAnnotator extends Annotator {
this.createPlainHighlight = this.createPlainHighlight.bind(this);
// $FlowFixMe
this.onSelectionChange = this.onSelectionChange.bind(this);
// $FlowFixMe
this.resetAnnotationUI = this.resetAnnotationUI.bind(this);
}

/**
Expand Down Expand Up @@ -240,26 +242,21 @@ class DocAnnotator extends Annotator {
return location;
};

/**
* Override to factor in highlight types being filtered out, if disabled. Also scales annotation canvases.
*
* @param {number} pageNum Page number
* @return {void}
*/
renderPage(pageNum: number) {
/** @inheritdoc */
resetAnnotationUI(pageNum?: number) {
// $FlowFixMe
document.getSelection().removeAllRanges();
if (this.highlighter) {
this.highlighter.removeAllHighlights();
}

// Scale existing canvases on re-render
this.scaleAnnotationCanvases(pageNum);
super.renderPage(pageNum);

if (this.createHighlightDialog) {
this.createHighlightDialog.unmountPopover();
}

if (pageNum) {
this.scaleAnnotationCanvases(pageNum);
}
}

/**
Expand Down Expand Up @@ -331,6 +328,8 @@ class DocAnnotator extends Annotator {
bindDOMListeners() {
super.bindDOMListeners();

this.container.addEventListener('resize', this.resetAnnotationUI);

// Highlight listeners on desktop & mobile
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
this.annotatedElement.addEventListener('wheel', this.hideCreateDialog);
Expand Down Expand Up @@ -365,6 +364,7 @@ class DocAnnotator extends Annotator {
unbindDOMListeners() {
super.unbindDOMListeners();

this.container.removeEventListener('resize', this.resetAnnotationUI);
this.annotatedElement.removeEventListener('wheel', this.hideCreateDialog);
this.annotatedElement.removeEventListener('touchend', this.hideCreateDialog);
this.annotatedElement.removeEventListener('click', this.clickHandler);
Expand Down
31 changes: 30 additions & 1 deletion src/doc/__tests__/CreateHighlightDialog-test.js
View file
Expand Up @@ -29,7 +29,6 @@ describe('doc/CreateHighlightDialog', () => {
});
dialog.annotatedElement = rootElement;
dialog.renderAnnotationPopover = jest.fn();
dialog.unmountPopover = jest.fn();
dialog.emit = jest.fn();

util.shouldDisplayMobileUI = jest.fn().mockReturnValue(false);
Expand Down Expand Up @@ -64,6 +63,36 @@ describe('doc/CreateHighlightDialog', () => {
});
});

describe('unmountPopover', () => {
beforeEach(() => {
dialog.isVisible = true;
dialog.createPopoverComponent = {};

dialog.container = {
querySelectorAll: jest.fn().mockReturnValue([rootElement])
};
});

it('should do nothing if the popover is not visible', () => {
dialog.isVisible = false;
dialog.unmountPopover();
expect(dialog.createPopoverComponent).not.toBeNull();
});

it('should there are no popover layers', () => {
dialog.container = {
querySelectorAll: jest.fn().mockReturnValue([])
};
dialog.unmountPopover();
expect(dialog.createPopoverComponent).not.toBeNull();
});

it('should unmount any visible create highlight popovers', () => {
dialog.unmountPopover();
expect(dialog.createPopoverComponent).toBeNull();
});
});

describe('show()', () => {
const selection = { anchorNode: {} };
const pageInfo = { page: 1, pageEl: {} };
Expand Down
43 changes: 19 additions & 24 deletions src/doc/__tests__/DocAnnotator-test.js
View file
Expand Up @@ -409,7 +409,7 @@ describe('doc/DocAnnotator', () => {
});
});

describe('renderPage()', () => {
describe('resetAnnotationUI()', () => {
beforeEach(() => {
document.getSelection = jest.fn().mockReturnValue({
removeAllRanges: jest.fn()
Expand All @@ -418,27 +418,17 @@ describe('doc/DocAnnotator', () => {
removeAllHighlights: jest.fn()
};
annotator.scaleAnnotationCanvases = jest.fn();
annotator.modeControllers = {
type: {
renderPage: jest.fn()
},
type2: {
renderPage: jest.fn()
}
};
});

it('should clear and hide createHighlightDialog on page render', () => {
annotator.createHighlightDialog = {
isVisible: true,
hide: jest.fn(),
destroy: jest.fn(),
unmountPopover: jest.fn()
};
annotator.renderPage(1);
it('should clear and hide createHighlightDialog', () => {
annotator.resetAnnotationUI();
expect(annotator.scaleAnnotationCanvases).not.toBeCalled();
expect(annotator.createHighlightDialog.unmountPopover).toBeCalled();
});

it('should scale annotation canvases if page number is provided', () => {
annotator.resetAnnotationUI(1);
expect(annotator.scaleAnnotationCanvases).toBeCalledWith(1);
expect(annotator.modeControllers.type.renderPage).toBeCalledWith(1);
expect(annotator.modeControllers.type2.renderPage).toBeCalledWith(1);
expect(annotator.createHighlightDialog.unmountPopover).toBeCalled();
});
});
Expand Down Expand Up @@ -478,10 +468,6 @@ describe('doc/DocAnnotator', () => {
Object.defineProperty(Annotator.prototype, 'setupAnnotations', { value: jest.fn() });
rangy.createClassApplier = jest.fn();
rangy.createHighlighter = jest.fn().mockReturnValue(highlighter);

annotator.createHighlightDialog = {
addListener: jest.fn()
};
});

afterEach(() => {
Expand Down Expand Up @@ -518,6 +504,10 @@ describe('doc/DocAnnotator', () => {

beforeEach(() => {
Object.defineProperty(Annotator.prototype, 'bindDOMListeners', { value: jest.fn() });
annotator.container = {
addEventListener: jest.fn(),
removeEventListener: jest.fn()
};
annotator.annotatedElement = {
addEventListener: jest.fn(),
removeEventListener: jest.fn()
Expand All @@ -535,6 +525,7 @@ describe('doc/DocAnnotator', () => {

it('should bind DOM listeners if user can annotate and highlight', () => {
annotator.bindDOMListeners();
expect(annotator.container.addEventListener).toBeCalledWith('resize', annotator.resetAnnotationUI);
expect(annotator.annotatedElement.addEventListener).toBeCalledWith(
'mouseup',
annotator.highlightMouseupHandler
Expand Down Expand Up @@ -628,6 +619,9 @@ describe('doc/DocAnnotator', () => {

describe('unbindDOMListeners()', () => {
beforeEach(() => {
annotator.container = {
removeEventListener: jest.fn()
};
annotator.annotatedElement = {
removeEventListener: jest.fn()
};
Expand All @@ -640,6 +634,7 @@ describe('doc/DocAnnotator', () => {
annotator.permissions.can_annotate = true;

annotator.unbindDOMListeners();
expect(annotator.container.removeEventListener).toBeCalledWith('resize', annotator.resetAnnotationUI);
expect(annotator.annotatedElement.removeEventListener).toBeCalledWith(
'mouseup',
annotator.highlightMouseupHandler
Expand Down Expand Up @@ -967,7 +962,7 @@ describe('doc/DocAnnotator', () => {

it('should do nothing and return true if a createHighlightDialog is visible', () => {
annotator.plainHighlightEnabled = true;
annotator.createHighlightDialog = { isVisible: true };
annotator.createHighlightDialog.isVisible = true;
expect(annotator.highlightClickHandler(event)).toBeTruthy();
expect(annotator.hideAnnotations).not.toBeCalled();
});
Expand Down
8 changes: 4 additions & 4 deletions src/drawing/DrawingThread.js
View file
Expand Up @@ -163,13 +163,13 @@ class DrawingThread extends AnnotationThread {
this.annotatedElement.removeEventListener('touchend', eventToLocationHandler);
}

/* eslint-disable no-unused-vars */
/**
* Handle a pointer movement
*
* @param {Object} location - The location information of the pointer
* @return {void}
*/
/* eslint-disable-next-line no-unused-vars */
handleMove(location) {}

/**
Expand All @@ -178,6 +178,7 @@ class DrawingThread extends AnnotationThread {
* @param {Object} location - The location information of the pointer
* @return {void}
*/
/* eslint-disable-next-line no-unused-vars */
handleStart(location) {}

/**
Expand All @@ -186,8 +187,8 @@ class DrawingThread extends AnnotationThread {
* @param {Object} location - The location information of the pointer
* @return {void}
*/
/* eslint-disable-next-line no-unused-vars */
handleStop(location) {}
/* eslint-disable no-unused-vars */

/**
* Delete a saved drawing thread by deleting each annotation
Expand Down Expand Up @@ -430,10 +431,9 @@ class DrawingThread extends AnnotationThread {
* Create an annotation data object to pass to annotation service.
*
* @param {string} type - Type of annotation
* @param {string} message - Annotation text
* @return {Object} Annotation data
*/
createAnnotationData(type, message) {
createAnnotationData(type) {
return {
item: {
id: this.fileVersionId,
Expand Down

0 comments on commit 43e5cb0

Please sign in to comment.