diff --git a/src/Annotator.scss b/src/Annotator.scss index 353a76c7f..fbdbb18f5 100644 --- a/src/Annotator.scss +++ b/src/Annotator.scss @@ -168,7 +168,8 @@ // Quad point positioning - the helper divs are positioned relative to the Rangy-created element .bp-doc .rangy-highlight { - background-color: #ff0; + background-color: lighten($highlight-yellow, 10%); + opacity: .25; position: relative; } diff --git a/src/components/ActionControls/ActionControls.scss b/src/components/ActionControls/ActionControls.scss index 8edb65eff..73518ba47 100644 --- a/src/components/ActionControls/ActionControls.scss +++ b/src/components/ActionControls/ActionControls.scss @@ -21,14 +21,14 @@ margin: 0; } - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw, - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight { + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw, + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight { border-left: 1px solid $see-see; margin-left: 5px; } // TABLET-SPECIFIC CSS - @media #{$tablet} { + @media #{$mobile}, #{$tablet} { .ba-create-popover .ba-action-controls { border: none; box-shadow: none; @@ -38,9 +38,10 @@ width: 100%; } - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw, - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight { + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw, + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight { border-left: 0; + margin: 0; } } diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index 8111f0979..1383f68f8 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -2,11 +2,11 @@ import React from 'react'; import classNames from 'classnames'; import noop from 'lodash/noop'; -import Overlay from 'box-react-ui/lib/components/flyout/Overlay'; import PlainButton from 'box-react-ui/lib/components/plain-button'; import IconClose from 'box-react-ui/lib/icons/general/IconClose'; import Internationalize from '../Internationalize'; +import Overlay from './Overlay'; import CommentList from '../CommentList'; import { TYPES, CLASS_ANNOTATION_POPOVER } from '../../constants'; @@ -93,8 +93,7 @@ class AnnotationPopover extends React.PureComponent { ) : ( )} - - + {hasComments ? ( ) : ( diff --git a/src/components/AnnotationPopover/AnnotatorLabel.js b/src/components/AnnotationPopover/AnnotatorLabel.js index 87dc0e7b9..8c2805c54 100644 --- a/src/components/AnnotationPopover/AnnotatorLabel.js +++ b/src/components/AnnotationPopover/AnnotatorLabel.js @@ -42,7 +42,7 @@ class AnnotatorLabel extends React.PureComponent { const { id, isPending } = this.props; return ( !isPending && ( - + ) diff --git a/src/components/AnnotationPopover/AnnotatorLabel.scss b/src/components/AnnotationPopover/AnnotatorLabel.scss index 071522775..b6bf83f91 100644 --- a/src/components/AnnotationPopover/AnnotatorLabel.scss +++ b/src/components/AnnotationPopover/AnnotatorLabel.scss @@ -1,15 +1,17 @@ @import '../../variables'; .ba { - .ba-annotation-label { + .ba-annotator-label { line-height: 23px; } // MOBILE & TABLET CSS @media #{$mobile}, #{$tablet} { - .ba-annotation-label { + .ba-annotator-label { background: white; border-bottom: 1px solid #ccc; + display: flex; + justify-content: center; left: 0; padding: 20px; position: fixed; diff --git a/src/components/AnnotationPopover/Overlay.js b/src/components/AnnotationPopover/Overlay.js new file mode 100644 index 000000000..121ca6924 --- /dev/null +++ b/src/components/AnnotationPopover/Overlay.js @@ -0,0 +1,23 @@ +// @flow +import * as React from 'react'; + +import OverlayComponent from 'box-react-ui/lib/components/flyout/Overlay'; + +type Props = { + shouldDefaultFocus: boolean, + children: React.Node +}; + +const Overlay = ({ shouldDefaultFocus, children }: Props) => { + if (shouldDefaultFocus) { + return ( + + {children} + + ); + } + + return
{children}
; +}; + +export default Overlay; diff --git a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js index 8112d6799..b12c0ffa0 100644 --- a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js +++ b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js @@ -73,6 +73,26 @@ describe('components/AnnotationPopover', () => { expect(wrapper.find('.ba-inline').length).toEqual(1); }); + test('should correctly render a BRUI Overlay if not on mobile', () => { + const wrapper = render({ + canAnnotate: true, + comments, + isMobile: false + }); + expect(wrapper).toMatchSnapshot(); + expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeTruthy(); + }); + + test('should correctly render a div without a Focus Trap if on mobile', () => { + const wrapper = render({ + canAnnotate: true, + comments, + isMobile: true + }); + expect(wrapper).toMatchSnapshot(); + expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeFalsy(); + }); + test('should correctly render a popover with comments and reply textarea', () => { const wrapper = render({ canAnnotate: true, diff --git a/src/components/AnnotationPopover/__tests__/Overlay-test.js b/src/components/AnnotationPopover/__tests__/Overlay-test.js new file mode 100644 index 000000000..aa2cb5076 --- /dev/null +++ b/src/components/AnnotationPopover/__tests__/Overlay-test.js @@ -0,0 +1,18 @@ +import * as React from 'react'; +import { shallow } from 'enzyme'; + +import Overlay from '../Overlay'; + +describe('components/Overlay', () => { + test('should correctly render a Focus Trap if not on mobile', () => { + const wrapper = shallow().dive(); + expect(wrapper).toMatchSnapshot(); + expect(wrapper.find('FocusTrap').length).toEqual(1); + }); + + test('should correctly render a div without a Focus Trap if on mobile', () => { + const wrapper = shallow(); + expect(wrapper).toMatchSnapshot(); + expect(wrapper.find('FocusTrap').length).toEqual(0); + }); +}); diff --git a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap index 23702f89b..e6761bc9b 100644 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap +++ b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap @@ -1,5 +1,242 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`components/AnnotationPopover should correctly render a BRUI Overlay if not on mobile 1`] = ` + +
+ + + + + +
+ +`; + +exports[`components/AnnotationPopover should correctly render a div without a Focus Trap if on mobile 1`] = ` + +
+ + + + + + + + + +
+ +`; + exports[`components/AnnotationPopover should correctly render a pending annotation 1`] = `
+
+ +`; + +exports[`components/Overlay should correctly render a div without a Focus Trap if on mobile 1`] = ` +
+`; diff --git a/src/components/CommentList/CommentList.scss b/src/components/CommentList/CommentList.scss index 2bddeabd3..1fd997f58 100644 --- a/src/components/CommentList/CommentList.scss +++ b/src/components/CommentList/CommentList.scss @@ -22,10 +22,15 @@ padding-top: 15px; } - .bcs-comment { + .bcs-comment, + .bcs-comment-user-name ~ .bcs-comment-created-at { margin: 0; } + .bcs-comment-content { + text-align: left; + } + // MOBILE CSS @media #{$mobile}, #{$tablet} { .ba-comment-list { diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index de692882a..17acbc047 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -211,12 +211,19 @@ class AnnotationModeController extends EventEmitter { this.emit(CONTROLLER_EVENT.toggleMode); } + /** + * @return {void} + */ + resetMode() {} + /** * Disables the specified annotation mode * * @return {void} */ exit(): void { + this.resetMode(); + if (this.currentThread) { this.currentThread.unmountPopover(); } diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index c0c504500..4f88e858e 100644 --- a/src/controllers/DrawingModeController.js +++ b/src/controllers/DrawingModeController.js @@ -229,7 +229,7 @@ class DrawingModeController extends AnnotationModeController { } /** @inheritdoc */ - exit() { + resetMode() { if (this.currentThread) { this.currentThread.clearBoundary(); } @@ -245,7 +245,6 @@ class DrawingModeController extends AnnotationModeController { pageElements.forEach((pageEl) => clearCanvas(pageEl, CLASS_ANNOTATION_LAYER_DRAW_IN_PROGRESS)); this.annotatedElement.classList.remove(CLASS_ANNOTATION_DRAW_MODE); - super.exit(); } /** diff --git a/src/controllers/HighlightModeController.js b/src/controllers/HighlightModeController.js index 6fab65202..2ce7a8264 100644 --- a/src/controllers/HighlightModeController.js +++ b/src/controllers/HighlightModeController.js @@ -34,7 +34,7 @@ class HighlightModeController extends AnnotationModeController { } /** @inheritdoc */ - exit(): void { + resetMode(): void { this.destroyPendingThreads(); window.getSelection().removeAllRanges(); this.unbindListeners(); // Disable mode diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js index 674787336..e7aaf8449 100644 --- a/src/controllers/PointModeController.js +++ b/src/controllers/PointModeController.js @@ -57,7 +57,7 @@ class PointModeController extends AnnotationModeController { } /** @inheritdoc */ - exit(): void { + resetMode(): void { if (this.buttonEl) { this.buttonEl.classList.remove(CLASS_ACTIVE); } @@ -70,7 +70,6 @@ class PointModeController extends AnnotationModeController { } this.pendingThreadID = null; - super.exit(); } /** @inheritdoc */ diff --git a/src/controllers/__tests__/HighlightModeController-test.js b/src/controllers/__tests__/HighlightModeController-test.js index c014ecb95..46940194d 100644 --- a/src/controllers/__tests__/HighlightModeController-test.js +++ b/src/controllers/__tests__/HighlightModeController-test.js @@ -62,7 +62,7 @@ describe('controllers/HighlightModeController', () => { }); }); - describe('exit()', () => { + describe('resetMode()', () => { it('should exit annotation mode', () => { controller.destroyPendingThreads = jest.fn(); controller.unbindListeners = jest.fn(); @@ -75,7 +75,7 @@ describe('controllers/HighlightModeController', () => { controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); - controller.exit(); + controller.resetMode(); expect(controller.destroyPendingThreads).toBeCalled(); expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.bindDOMListeners); expect(controller.unbindListeners).toBeCalled(); diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js index 4aebe6fa7..3fc8a6e0f 100644 --- a/src/controllers/__tests__/PointModeController-test.js +++ b/src/controllers/__tests__/PointModeController-test.js @@ -90,11 +90,8 @@ describe('controllers/PointModeController', () => { }); }); - describe('exit()', () => { + describe('resetMode()', () => { beforeEach(() => { - controller.destroyPendingThreads = jest.fn(); - controller.unbindListeners = jest.fn(); - // Set up annotation mode controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); @@ -104,19 +101,12 @@ describe('controllers/PointModeController', () => { controller.buttonEl.classList.add(CLASS_ACTIVE); }); - it('should exit annotation mode', () => { - controller.exit(); - expect(controller.destroyPendingThreads).toBeCalled(); - expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.exit, expect.any(Object)); - expect(controller.unbindListeners).toBeCalled(); - expect(controller.hadPendingThreads).toBeFalsy(); - }); - - it('should deactive mode button if available', () => { + it('should reset annotation mode', () => { controller.buttonEl = document.createElement('button'); controller.buttonEl.classList.add(CLASS_ACTIVE); - controller.exit(); + controller.resetMode(); expect(controller.buttonEl.classList).not.toContain(CLASS_ACTIVE); + expect(controller.hadPendingThreads).toBeFalsy(); }); }); diff --git a/src/doc/CreateHighlightDialog.js b/src/doc/CreateHighlightDialog.js index 6b860905b..35be6c9a9 100644 --- a/src/doc/CreateHighlightDialog.js +++ b/src/doc/CreateHighlightDialog.js @@ -90,11 +90,11 @@ class CreateHighlightDialog extends EventEmitter { /** * Render the popover * - * @param {HTMLElement} selection Current text selection + * @param {Selection} selection Current text selection * @param {AnnotationType} type - highlight type * @return {void} */ - show(selection: HTMLElement, type: AnnotationType = TYPES.highlight) { + show(selection: ?Selection, type: AnnotationType = TYPES.highlight) { if (!selection) { return; } @@ -265,6 +265,9 @@ class CreateHighlightDialog extends EventEmitter { onCommentClick = () => { this.emit(CREATE_EVENT.comment); this.renderAnnotationPopover(TYPES.highlight_comment); + + // $FlowFixMe + document.getSelection().removeAllRanges(); }; /** diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 51c46a514..2d2dd0af4 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -50,7 +50,7 @@ class DocAnnotator extends Annotator { lastHighlightEvent: ?Event; /** @property {Selection} - For tracking diffs in text selection, for mobile highlights creation. */ - lastSelection: ?HTMLElement; + lastSelection: ?Selection; /** @property {boolean} - True if regular highlights are allowed to be read/written */ plainHighlightEnabled: boolean; @@ -74,6 +74,8 @@ class DocAnnotator extends Annotator { this.createHighlightThread = this.createHighlightThread.bind(this); // $FlowFixMe this.createPlainHighlight = this.createPlainHighlight.bind(this); + // $FlowFixMe + this.onSelectionChange = this.onSelectionChange.bind(this); } /** @@ -438,6 +440,7 @@ class DocAnnotator extends Annotator { return; } + // $FlowFixMe this.createHighlightDialog.unmountPopover(); } @@ -487,9 +490,6 @@ class DocAnnotator extends Annotator { const location = this.getLocationFromEvent(this.lastHighlightEvent, highlightType); const controller = this.modeControllers[highlightType]; - this.highlighter.removeAllHighlights(); - this.resetHighlightSelection(this.lastHighlightEvent); - if (!location || !controller) { return null; } @@ -523,7 +523,7 @@ class DocAnnotator extends Annotator { * @param {Event} event The DOM event coming from interacting with the element. * @return {void} */ - onSelectionChange = (event: Event) => { + onSelectionChange(event: Event) { event.preventDefault(); event.stopPropagation(); @@ -560,7 +560,7 @@ class DocAnnotator extends Annotator { this.selectionEndTimeout = setTimeout(() => { if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { - this.createHighlightDialog.show(selection); + this.createHighlightDialog.show(this.lastSelection); } }, SELECTION_TIMEOUT); @@ -583,7 +583,7 @@ class DocAnnotator extends Annotator { } this.lastHighlightEvent = mouseEvent; this.lastSelection = selection; - }; + } /** * Mode controllers setup. @@ -617,7 +617,7 @@ class DocAnnotator extends Annotator { return; } - this.highlighter.highlightSelection('rangy-highlight', { + this.highlighter.highlightSelection(CLASS_RANGY_HIGHLIGHT, { containerElementId: this.annotatedElement.id }); } diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index b405cc3a8..12f76ea75 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -125,11 +125,10 @@ class DocHighlightThread extends AnnotationThread { delete(annotation: Object, useServer: boolean = true): Promise { const promise = super.delete(annotation, useServer); - if (!this.threadID) { - return promise; + if (this.threadID) { + this.renderAnnotationPopover(); } - this.renderAnnotationPopover(); return promise; } diff --git a/src/doc/__tests__/CreateHighlightDialog-test.js b/src/doc/__tests__/CreateHighlightDialog-test.js index 589f3f421..d6225867e 100644 --- a/src/doc/__tests__/CreateHighlightDialog-test.js +++ b/src/doc/__tests__/CreateHighlightDialog-test.js @@ -158,6 +158,9 @@ describe('doc/CreateHighlightDialog', () => { describe('onCommentClick()', () => { it('should create a plain highlight and render the popover', () => { + document.getSelection = jest.fn().mockReturnValue({ + removeAllRanges: jest.fn() + }); dialog.onCommentClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() }); expect(dialog.emit).toBeCalledWith(CREATE_EVENT.comment); expect(dialog.renderAnnotationPopover).toBeCalled();