From 1c6b4bdac2418eefea84014e859482168adec094 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 10:01:41 -0800 Subject: [PATCH 1/5] Chore: Add escape hotkey on AnnotationPopover --- package.json | 1 + src/AnnotationThread.js | 19 ---- .../AnnotationPopover/AnnotationPopover.js | 90 +++++++++++-------- src/doc/DocHighlightThread.js | 15 ---- yarn.lock | 5 ++ 5 files changed, 58 insertions(+), 72 deletions(-) diff --git a/package.json b/package.json index c762e89be..168bd8826 100644 --- a/package.json +++ b/package.json @@ -96,6 +96,7 @@ "lint-staged": "^7.2.2", "mini-css-extract-plugin": "^0.4.2", "mocha": "^5.2.0", + "mousetrap": "^1.6.2", "node-sass": "^4.9.3", "optimize-css-assets-webpack-plugin": "^4.0.2", "pikaday": "^1.8.0", diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index 6e884c639..51156ef16 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -669,25 +669,6 @@ class AnnotationThread extends EventEmitter { super.emit('threadevent', { event, data: threadData, eventData }); } - /** - * Keydown handler for dialog. - * - * @param {Event} event DOM event - * @return {void} - */ - keydownHandler(event: Event) { - event.stopPropagation(); - - const key = util.decodeKeydown(event); - if (key === 'Escape') { - if (this.hasAnnotations()) { - this.hide(); - } else { - this.cancelAnnotation(); - } - } - } - /** * Show/hide the top portion of the annotations icon based on if the * entire dialog is flipped diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index 0ab916f35..1157858e5 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -4,6 +4,7 @@ import classNames from 'classnames'; import noop from 'lodash/noop'; import PlainButton from 'box-react-ui/lib/components/plain-button'; import IconClose from 'box-react-ui/lib/icons/general/IconClose'; +import { HotkeyRecord, HotkeyLayer } from 'box-react-ui/lib/components/hotkeys'; import Internationalize from '../Internationalize'; import CommentList from '../CommentList'; @@ -43,7 +44,6 @@ class AnnotationPopover extends React.PureComponent { canDelete: false, onCommentClick: noop, onDelete: noop, - onCancel: noop, onCreate: noop, comments: [] }; @@ -58,6 +58,18 @@ class AnnotationPopover extends React.PureComponent { position(); } + getHotkeyConfigs() { + const { onCancel } = this.props; + return [ + new HotkeyRecord({ + description: 'Close popover', + key: 'esc', + handler: () => onCancel(), + type: 'Close' + }) + ]; + } + render() { const { id, @@ -83,46 +95,48 @@ class AnnotationPopover extends React.PureComponent { return ( -
- {isMobile ? ( - - - - - - ) : ( - - )} -
- {hasComments ? ( - + +
+ {isMobile ? ( + + + + + ) : ( - - )} - {canAnnotate && ( - + )} +
+ {hasComments ? ( + + ) : ( + + )} + {canAnnotate && ( + + )} +
-
+ ); } diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index c01d56b35..77b98de64 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -498,21 +498,6 @@ class DocHighlightThread extends AnnotationThread { popoverEl.style.top = `${dialogY + INLINE_POPOVER_HEIGHT / 2 - BORDER_OFFSET}px`; }; - /** - * Keydown handler on dialog. Needed since we are binding to 'mousedown' - * instead of 'click'. - * - * @param {Event} event - Mouse event - * @return {void} - */ - keydownHandler(event: Event) { - event.stopPropagation(); - if (util.decodeKeydown(event) === 'Enter') { - this.mousedownHandler(event); - } - super.keydownHandler(event); - } - /** @inheritdoc */ cleanupAnnotationOnDelete(annotationIDToRemove: string) { // Delete matching comment from annotation diff --git a/yarn.lock b/yarn.lock index 88b53e57c..f991579a9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8668,6 +8668,11 @@ moo@^0.4.3: resolved "https://registry.yarnpkg.com/moo/-/moo-0.4.3.tgz#3f847a26f31cf625a956a87f2b10fbc013bfd10e" integrity sha512-gFD2xGCl8YFgGHsqJ9NKRVdwlioeW3mI1iqfLNYQOv0+6JRwG58Zk9DIGQgyIaffSYaO1xsKnMaYzzNr1KyIAw== +mousetrap@^1.6.2: + version "1.6.2" + resolved "https://registry.yarnpkg.com/mousetrap/-/mousetrap-1.6.2.tgz#caadd9cf886db0986fb2fee59a82f6bd37527587" + integrity sha512-jDjhi7wlHwdO6q6DS7YRmSHcuI+RVxadBkLt3KHrhd3C2b+w5pKefg3oj5beTcHZyVFA9Aksf+yEE1y5jxUjVA== + move-concurrently@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/move-concurrently/-/move-concurrently-1.0.1.tgz#be2c005fda32e0b29af1f05d7c4b33214c701f92" From 8ab7484e7a4b236063a9fb297d51899aad9cb33d Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 13:58:16 -0800 Subject: [PATCH 2/5] Chore: Remove anonymous function --- src/components/AnnotationPopover/AnnotationPopover.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index 1157858e5..f7a4fbc5a 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -64,7 +64,7 @@ class AnnotationPopover extends React.PureComponent { new HotkeyRecord({ description: 'Close popover', key: 'esc', - handler: () => onCancel(), + handler: onCancel, type: 'Close' }) ]; From 51fc184fefcf338db139b40f787e12aeff5be64e Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 15:29:21 -0800 Subject: [PATCH 3/5] Update: tests --- .../AnnotationPopover/AnnotationPopover.js | 22 +- .../AnnotationPopover-test.js.snap | 824 +++++++++++------- 2 files changed, 496 insertions(+), 350 deletions(-) diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index f7a4fbc5a..e3ed156b8 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -58,18 +58,6 @@ class AnnotationPopover extends React.PureComponent { position(); } - getHotkeyConfigs() { - const { onCancel } = this.props; - return [ - new HotkeyRecord({ - description: 'Close popover', - key: 'esc', - handler: onCancel, - type: 'Close' - }) - ]; - } - render() { const { id, @@ -92,10 +80,18 @@ class AnnotationPopover extends React.PureComponent { } = this.props; const hasComments = comments.length > 0; const isInline = !hasComments && (type === TYPES.highlight || type === TYPES.draw); + const configs = [ + new HotkeyRecord({ + description: 'Close popover', + key: 'esc', + handler: onCancel, + type: 'Close' + }) + ]; return ( - +
-
- - - - - + }, + "key": "esc", + "type": "Close", + }, + ] + } + enableHelpModal={false} + helpModalShortcut="?" + >
- - + + + + +
+ + onDelete={ + [MockFunction] { + "calls": Array [ + Array [], + ], + "results": Array [ + Object { + "isThrow": false, + "value": undefined, + }, + ], + } + } + /> + +
-
+ `; exports[`components/AnnotationPopover should correctly render a pending annotation 1`] = ` -
- -
- - +
+ +
+ + + /> +
-
+ `; exports[`components/AnnotationPopover should correctly render a popover with comments and reply textarea 1`] = ` -
- -
- +
+ - + + /> + +
-
+ `; exports[`components/AnnotationPopover should correctly render a view-only popover with comments 1`] = ` -
- -
- +
+ +
+ +
-
+ `; exports[`components/AnnotationPopover should correctly render an annotation with a annotator label and no comments 1`] = ` -
- -
- - +
+ +
+ + +
+
+ + +`; + +exports[`components/AnnotationPopover should render a view-only annotation with a annotator label and no comments 1`] = ` + + -
-
- -`; - -exports[`components/AnnotationPopover should render a view-only annotation with a annotator label and no comments 1`] = ` - -
-
- +
+ +
-
+ `; From fd59ce1e315cb5163c8ccf47891eb5bda211645c Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 15:34:14 -0800 Subject: [PATCH 4/5] Chore: Add localization --- .../AnnotationPopover/AnnotationPopover.js | 4 ++- .../AnnotationPopover-test.js.snap | 30 +++++++++++++++---- src/components/AnnotationPopover/messages.js | 5 ++++ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index e3ed156b8..a262d188b 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -2,10 +2,12 @@ import React from 'react'; import classNames from 'classnames'; import noop from 'lodash/noop'; +import { FormattedMessage } from 'react-intl'; import PlainButton from 'box-react-ui/lib/components/plain-button'; import IconClose from 'box-react-ui/lib/icons/general/IconClose'; import { HotkeyRecord, HotkeyLayer } from 'box-react-ui/lib/components/hotkeys'; +import messages from './messages'; import Internationalize from '../Internationalize'; import CommentList from '../CommentList'; import { TYPES, CLASS_ANNOTATION_POPOVER, CLASS_ANNOTATION_CARET } from '../../constants'; @@ -82,7 +84,7 @@ class AnnotationPopover extends React.PureComponent { const isInline = !hasComments && (type === TYPES.highlight || type === TYPES.draw); const configs = [ new HotkeyRecord({ - description: 'Close popover', + description: , key: 'esc', handler: onCancel, type: 'Close' diff --git a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap index e7b7a6d43..d2d255d99 100644 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap +++ b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap @@ -6,7 +6,10 @@ exports[`components/AnnotationPopover should correctly render a div without a Fo configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], @@ -163,7 +166,10 @@ exports[`components/AnnotationPopover should correctly render a pending annotati configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], @@ -253,7 +259,10 @@ exports[`components/AnnotationPopover should correctly render a popover with com configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], @@ -383,7 +392,10 @@ exports[`components/AnnotationPopover should correctly render a view-only popove configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], @@ -467,7 +479,10 @@ exports[`components/AnnotationPopover should correctly render an annotation with configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], @@ -559,7 +574,10 @@ exports[`components/AnnotationPopover should render a view-only annotation with configs={ Array [ Immutable.Record { - "description": "Close popover", + "description": , "handler": [MockFunction] { "calls": Array [ Array [], diff --git a/src/components/AnnotationPopover/messages.js b/src/components/AnnotationPopover/messages.js index 24dd65293..71a71da80 100644 --- a/src/components/AnnotationPopover/messages.js +++ b/src/components/AnnotationPopover/messages.js @@ -28,6 +28,11 @@ const messages: { [string]: MessageDescriptor } = defineMessages({ id: 'ba.whoAnnotated', description: 'Label for who left the annotation', defaultMessage: '{name} annotated' + }, + close: { + id: 'ba.closePopover', + description: 'Description for popover close hotkey record', + defaultMessage: 'Close Popover' } }); From 14c40874e2a6e525c1caa4ac900e81ae0ff4cd20 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 25 Jan 2019 10:20:00 -0800 Subject: [PATCH 5/5] Update: Translations --- i18n/en-US.properties | 2 ++ 1 file changed, 2 insertions(+) diff --git a/i18n/en-US.properties b/i18n/en-US.properties index d5ff763a6..1400fa7cf 100644 --- a/i18n/en-US.properties +++ b/i18n/en-US.properties @@ -1,5 +1,7 @@ # Placeholder when the current annotation's user information is unknown ba.anonymousUserName = Some User +# Description for popover close hotkey record +ba.closePopover = Close Popover # Label for who left the annotation ba.whoAnnotated = {name} annotated # Label for who drew the drawing annotation