diff --git a/src/app/content/highlights/components/CardWrapper.spec.tsx b/src/app/content/highlights/components/CardWrapper.spec.tsx index fafb81044f..eab560cb6a 100644 --- a/src/app/content/highlights/components/CardWrapper.spec.tsx +++ b/src/app/content/highlights/components/CardWrapper.spec.tsx @@ -138,7 +138,7 @@ describe('CardWrapper', () => { // Handle null value renderer.act(() => { - card1.props.onHeightChange({ current: { offsetHeight: null } }); + card1.props.onHeightChange({ current: null }); }); // First card have null height so secondcard starts at the highlight top offset expect(card1.props.topOffset).toEqual(100); @@ -332,6 +332,14 @@ describe('CardWrapper', () => { }); }); + renderer.act(() => { + // Simulate pressing Escape to hide card + dispatchKeyDownEvent({ + key: 'Escape', + target: highlightElement1, + }); + }); + // Expect cards to be visible again renderer.act(() => { // Simulate pressing Enter to unhide cards @@ -341,7 +349,6 @@ describe('CardWrapper', () => { }); }); - // Expect all cards to be visible renderer.act(() => { // Simulate pressing Tab to unhide all cards dispatchKeyDownEvent({ @@ -350,11 +357,22 @@ describe('CardWrapper', () => { }); }); - // Set focusedHighlight, and do double=click renderer.act(() => { - highlightElement1.dispatchEvent(new Event('focus', { bubbles: true })); - highlightElement1.dispatchEvent(new Event('dblclick', { bubbles: true })); + // Simulate pressing Escape to hide card + dispatchKeyDownEvent({ + key: 'Escape', + target: highlightElement1, + }); }); + + // Trigger editOnEnter with no focusedHighlight + renderer.act(() => { + dispatchKeyDownEvent({ + key: 'Enter', + }); + }); + + document?.dispatchEvent(new CustomEvent('showCardEvent', { bubbles: true })); }); it( diff --git a/src/app/content/highlights/components/CardWrapper.tsx b/src/app/content/highlights/components/CardWrapper.tsx index dc5b7e1d44..6ef48018a1 100644 --- a/src/app/content/highlights/components/CardWrapper.tsx +++ b/src/app/content/highlights/components/CardWrapper.tsx @@ -6,7 +6,7 @@ import ResizeObserver from 'resize-observer-polyfill'; import styled from 'styled-components'; import { isHtmlElement } from '../../../guards'; import { useFocusLost, useKeyCombination, useFocusHighlight } from '../../../reactUtils'; -import { AppState } from '../../../types'; +import { AppState, Dispatch } from '../../../types'; import { assertDefined, assertDocument } from '../../../utils'; import * as selectSearch from '../../search/selectors'; import * as contentSelect from '../../selectors'; @@ -23,6 +23,7 @@ export interface WrapperProps { highlighter: Highlighter; highlights: Highlight[]; className?: string; + dispatch: Dispatch; } function checkIfHiddenByCollapsedAncestor(highlight: Highlight) { @@ -87,12 +88,6 @@ function useCardsHeights() { return [cardsHeights, onHeightChange] as const; } -function rangeString({startOffset, endOffset}: Highlight['range']) { - return `${startOffset}-${endOffset}`; -} - -const ELAPSED_LIMIT = 100; - function useFocusedHighlight( highlights: Highlight[], element: React.RefObject, @@ -104,47 +99,23 @@ function useFocusedHighlight( [focusedId, highlights]); const [shouldFocusCard, setShouldFocusCard] = React.useState(false); const document = assertDocument(); - const previousRange = React.useRef(''); - const [dblclickStamp, setDblclickStamp] = React.useState(0); + // catches the "click here" event sent by the EditCard React.useEffect(() => { - const handler = () => setDblclickStamp(Date.now()); + const handler = () => setShouldFocusCard(true); - document.addEventListener('dblclick', handler); - return () => document.removeEventListener('dblclick', handler); + document.addEventListener('showCardEvent', handler); + return () => document.removeEventListener('showCardEvent', handler); }, [document]); - // Tracking double clicks - // double-click events trigger updates to focusedHighlight, but the order and - // timing of processing can vary, so we check conditions within a short time - // of a double click. + // Ensure focusedHighlight is actually focused React.useEffect(() => { - if (focusedHighlight) { - const elapsed = Date.now() - dblclickStamp; - const isDoubleClick = elapsed < ELAPSED_LIMIT; - - // Existing highlight - if (focusedHighlight.elements.length > 0) { - if (isDoubleClick) { - // Unselect text inside existing highlight. - document.getSelection()?.removeAllRanges(); - setShouldFocusCard(true); - } - return; - } - - // Text selection that could be a highlight - const newRange = rangeString(focusedHighlight.range); - const isExistingSelection = newRange === previousRange.current; - - if (isExistingSelection && isDoubleClick) { - setShouldFocusCard(true); - } - previousRange.current = newRange; + if (focusedHighlight && focusedHighlight.elements.length > 0) { + focusedHighlight?.focus(); } - }, [focusedHighlight, document, dblclickStamp]); + }, [focusedHighlight]); - // Let Enter go from a highlight to the editor + // Pressing Enter moves the users from a highlight to the editor const editOnEnter = React.useCallback(() => { if (focusedHighlight) { setShouldFocusCard(true); @@ -163,20 +134,23 @@ function useFocusedHighlight( setShouldFocusCard(!cardIsFocused); }, [element, focusedHighlight]); - useKeyCombination({key: 'Enter'}, editOnEnter); + useKeyCombination({key: 'Enter'}, editOnEnter, noopKeyCombinationHandler([container, element])); useKeyCombination(highlightKeyCombination, moveFocus, noopKeyCombinationHandler([container, element])); // Clear shouldFocusCard when focus is lost from the CardWrapper. // If we don't do this then card related for the focused highlight will be focused automatically. useFocusLost(element, shouldFocusCard, React.useCallback(() => setShouldFocusCard(false), [])); - return [focusedHighlight, shouldFocusCard] as const; + return [focusedHighlight, shouldFocusCard, setShouldFocusCard] as const; } -function CardsForHighlights({highlights, container, focusedHighlight, shouldFocusCard, highlighter}: { +function CardsForHighlights({ + highlights, container, focusedHighlight, shouldFocusCard, setShouldFocusCard, highlighter, +}: { highlights: Highlight[]; container: HTMLElement; focusedHighlight: Highlight | undefined; shouldFocusCard: boolean; + setShouldFocusCard: (v: boolean) => void; highlighter: Highlighter; }) { const [cardsHeights, onHeightChange] = useCardsHeights(); @@ -190,9 +164,18 @@ function CardsForHighlights({highlights, container, focusedHighlight, shouldFocu editCardVisibilityHandler, new Map(highlights.map((highlight) => [highlight.id, false])) ); + + // First time, Esc closes it to the instructions; second Esc disappears it const hideCard = () => { + if (!focusedHighlight?.elements.length) { + return; + } focusedHighlight?.focus(); - dispatch({ type: 'HIDE', id: focusedHighlight?.id }); + if (shouldFocusCard) { + setShouldFocusCard(false); + } else { + dispatch({ type: 'HIDE', id: focusedHighlight?.id }); + } }; const showCard = (cardId: string | undefined) => { dispatch({ type: 'SHOW', id: cardId }); @@ -228,7 +211,7 @@ function CardsForHighlights({highlights, container, focusedHighlight, shouldFocu // tslint:disable-next-line:variable-name const Wrapper = ({highlights, className, container, highlighter}: WrapperProps) => { const element = React.useRef(null); - const [focusedHighlight, shouldFocusCard] = useFocusedHighlight(highlights, element, container); + const [focusedHighlight, shouldFocusCard, setShouldFocusCard] = useFocusedHighlight(highlights, element, container); return
; diff --git a/src/app/content/highlights/components/EditCard.spec.tsx b/src/app/content/highlights/components/EditCard.spec.tsx index 7b5f1de451..d4999eeaf3 100644 --- a/src/app/content/highlights/components/EditCard.spec.tsx +++ b/src/app/content/highlights/components/EditCard.spec.tsx @@ -2,6 +2,7 @@ import { Highlight } from '@openstax/highlighter'; import { HighlightUpdateColorEnum } from '@openstax/highlighter/dist/api'; import React, { ReactElement } from 'react'; import renderer from 'react-test-renderer'; +import ReactTestUtils from 'react-dom/test-utils'; import createTestServices from '../../../../test/createTestServices'; import createTestStore from '../../../../test/createTestStore'; import createMockHighlight from '../../../../test/mocks/highlight'; @@ -101,6 +102,27 @@ describe('EditCard', () => { const tree = component.toJSON(); expect(tree).toMatchSnapshot(); + + mockSpyUser.mockClear(); + }); + + it('shows create highlight message for new highlight', () => { + const newHighlight = {...highlight, elements: []}; + const mockSpyUser = jest.spyOn(selectAuth, 'user') + .mockReturnValue(formatUser(testAccountsUser)); + const component = renderer.create( + + + + ); + + const button = component.root.findByType('button'); + expect(button.props.children.props.id).toBe('i18n:highlighting:create-instructions'); mockSpyUser.mockClear(); }); @@ -629,7 +651,7 @@ describe('EditCard', () => { ...highlightData, }; - renderToDom( + const component = renderToDom(
text @@ -650,6 +672,17 @@ describe('EditCard', () => { document?.querySelector('a')?.focus(); document?.getElementById(MAIN_CONTENT_ID)?.focus(); expect(editCardProps.onBlur).not.toHaveBeenCalled(); + const button = component.node.querySelector('button') as HTMLButtonElement; + const preventDefault = jest.fn(); + document!.dispatchEvent = jest.fn(); + + // Two branches of showCard - must be mousedown of button 0 + ReactTestUtils.Simulate.mouseDown(button, { preventDefault, button: 1 }); + expect(preventDefault).not.toHaveBeenCalled(); + ReactTestUtils.Simulate.mouseDown(button, { preventDefault, button: 0 }); + expect(preventDefault).toHaveBeenCalled(); + expect(document!.dispatchEvent).toHaveBeenCalled(); + mockSpyUser.mockClear(); }); diff --git a/src/app/content/highlights/components/EditCard.tsx b/src/app/content/highlights/components/EditCard.tsx index 729df9738d..1816132e47 100644 --- a/src/app/content/highlights/components/EditCard.tsx +++ b/src/app/content/highlights/components/EditCard.tsx @@ -12,6 +12,7 @@ import { useFocusElement, useOnEsc, useTrapTabNavigation } from '../../../reactU import theme from '../../../theme'; import { assertDefined, assertWindow, mergeRefs } from '../../../utils'; import { highlightStyles } from '../../constants'; +import { cardWidth } from '../constants'; import defer from 'lodash/fp/defer'; import { clearFocusedHighlight, @@ -67,6 +68,13 @@ function LoginOrEdit({ const authenticated = !!useSelector(selectAuth.user); const element = React.useRef(null); const {formatMessage} = useIntl(); + const showCard = React.useCallback((event: React.MouseEvent) => { + if (event.button === 0) { + event.preventDefault(); + document?.dispatchEvent(new CustomEvent('showCardEvent', { bubbles: true })); + } + }, []); + const isNewSelection = props.highlight.elements.length === 0; return (
{ authenticated ? ( - (props.shouldFocusCard || props.data?.annotation) ? ( -
- - - ) : - Press Enter or double-click highlight to edit highlight + + { + (props.shouldFocusCard || props.data?.annotation) ? ( +
+ + + ) : + + } +
) : }
@@ -117,6 +135,7 @@ function LoginConfirmation({ // tslint:disable-next-line:variable-name const HiddenOnMobile = styled.div` + min-width: ${cardWidth}rem; ${theme.breakpoints.touchDeviceQuery(css` display: none; `)} @@ -205,7 +224,7 @@ function ActiveEditCard({ useTrapTabNavigation(ref, editingAnnotation); return ( - +
setConfirmingDelete(false)} /> )} - +
); } diff --git a/src/app/content/highlights/components/__snapshots__/Card.spec.tsx.snap b/src/app/content/highlights/components/__snapshots__/Card.spec.tsx.snap index 4b1662d209..631182a855 100644 --- a/src/app/content/highlights/components/__snapshots__/Card.spec.tsx.snap +++ b/src/app/content/highlights/components/__snapshots__/Card.spec.tsx.snap @@ -10,11 +10,10 @@ exports[`Card matches snapshot when focused without note 1`] = ` padding: 0.8rem; border-radius: 0.4rem; box-shadow: 0 0 2px 0 rgba(0,0,0,0.14),0 2px 2px 0 rgba(0,0,0,0.12),0 1px 3px 0 rgba(0,0,0,0.2); - left: unset; - right: 0; + left: calc(50% + (82.5rem / 2) + 3rem); + right: unset; top: 150px; left: calc(50% + (82.5rem / 2) + 1rem); - right: unset; -webkit-transition: opacity 0.3s,top 0.3s,left 0.3s; transition: opacity 0.3s,top 0.3s,left 0.3s; } @@ -44,8 +43,8 @@ exports[`Card matches snapshot when focused without note 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 0px; } } @@ -54,8 +53,8 @@ exports[`Card matches snapshot when focused without note 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 0px; } } @@ -129,11 +128,10 @@ exports[`Card matches snapshot when passed data without note 1`] = ` padding: 0.8rem; border-radius: 0.4rem; box-shadow: 0 0 2px 0 rgba(0,0,0,0.14),0 2px 2px 0 rgba(0,0,0,0.12),0 1px 3px 0 rgba(0,0,0,0.2); - left: unset; - right: 0; + left: calc(50% + (82.5rem / 2) + 3rem); + right: unset; top: undefinedpx; left: calc(50% + (82.5rem / 2) + 1rem); - right: unset; -webkit-transition: opacity 0.3s,top 0.3s,left 0.3s; transition: opacity 0.3s,top 0.3s,left 0.3s; } @@ -152,8 +150,8 @@ exports[`Card matches snapshot when passed data without note 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 0px; } } @@ -162,8 +160,8 @@ exports[`Card matches snapshot when passed data without note 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 0px; } } @@ -223,11 +221,10 @@ exports[`Card matches snapshot without data 1`] = ` padding: 0.8rem; border-radius: 0.4rem; box-shadow: 0 0 2px 0 rgba(0,0,0,0.14),0 2px 2px 0 rgba(0,0,0,0.12),0 1px 3px 0 rgba(0,0,0,0.2); - left: unset; - right: 0; + left: calc(50% + (82.5rem / 2) + 3rem); + right: unset; top: 200px; left: calc(50% + (82.5rem / 2) + 1rem); - right: unset; -webkit-transition: opacity 0.3s,top 0.3s,left 0.3s; transition: opacity 0.3s,top 0.3s,left 0.3s; } @@ -246,8 +243,8 @@ exports[`Card matches snapshot without data 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 200px; } } @@ -256,8 +253,8 @@ exports[`Card matches snapshot without data 1`] = ` .c0 { -webkit-animation: none; animation: none; - left: unset; - right: 2rem; + left: calc(75vw - (82.5rem / 2) + 1rem); + right: unset; top: 200px; } } diff --git a/src/app/content/highlights/components/__snapshots__/EditCard.spec.tsx.snap b/src/app/content/highlights/components/__snapshots__/EditCard.spec.tsx.snap index 67207cb732..01dc0242dd 100644 --- a/src/app/content/highlights/components/__snapshots__/EditCard.spec.tsx.snap +++ b/src/app/content/highlights/components/__snapshots__/EditCard.spec.tsx.snap @@ -1,7 +1,7 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`EditCard matches snapshot when editing 1`] = ` -.c3 { +.c4 { display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; @@ -32,20 +32,20 @@ exports[`EditCard matches snapshot when editing 1`] = ` -moz-osx-font-smoothing: grayscale; } -.c3:hover { +.c4:hover { background-color: ,#be3c08,; } -.c3:active { +.c4:active { background-color: ,#b03808,; } -.c3:focus { +.c4:focus { outline: solid #fff; box-shadow: inset 0 0 0 0.3rem #000; } -.c4 { +.c5 { display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; @@ -74,21 +74,25 @@ exports[`EditCard matches snapshot when editing 1`] = ` font-weight: regular; } -.c4:hover { +.c5:hover { background-color: ,#fafafa,; } -.c4:active { +.c5:active { background-color: ,#e5e5e5,; } -.c2 { +.c3 { display: grid; overflow: visible; grid-auto-flow: column; grid-gap: 1rem; } +.c1 { + min-width: 20rem; +} + .c0 { background: #f5f5f5; -webkit-user-select: none; @@ -98,7 +102,7 @@ exports[`EditCard matches snapshot when editing 1`] = ` overflow: visible; } -.c0 .c1 { +.c0 .c2 { margin-top: 0.8rem; } @@ -107,59 +111,61 @@ exports[`EditCard matches snapshot when editing 1`] = ` className="c0" role="dialog" > -
-
-
+
-
+
-
- - + + +
-
- + +
`; @@ -200,6 +206,10 @@ exports[`EditCard matches snapshot when focused 1`] = ` `; exports[`EditCard matches snapshot with data 1`] = ` +.c1 { + min-width: 20rem; +} + .c0 { background: #f5f5f5; -webkit-user-select: none; @@ -209,7 +219,7 @@ exports[`EditCard matches snapshot with data 1`] = ` overflow: visible; } -.c0 .c1 { +.c0 .c2 { margin-top: 0.8rem; } @@ -218,39 +228,41 @@ exports[`EditCard matches snapshot with data 1`] = ` className="c0" role="dialog" > -
-
-
+
-
+
-
- + mock-note="true" + /> +
+ +
`; diff --git a/src/app/content/highlights/components/cardStyles.ts b/src/app/content/highlights/components/cardStyles.ts index 32250fc115..645f244d97 100644 --- a/src/app/content/highlights/components/cardStyles.ts +++ b/src/app/content/highlights/components/cardStyles.ts @@ -48,8 +48,8 @@ export const minimalWidthForCardsWithSearchResults = '(max-width: ' + const overlapDisplay = css` ${(props: CardProps) => !!props.isActive && css` - left: unset; - right: ${cardMinWindowMargin}rem; + left: calc(75vw - (${contentTextWidth}rem / 2) + ${cardFocusedContentMargin}rem); + right: unset; top: ${props.highlightOffsets ? props.highlightOffsets.bottom : getHighlightBottomOffset(props.container, props.highlight)}px; @@ -60,12 +60,11 @@ const overlapDisplay = css` `; const rightSideDisplay = css` - left: unset; - right: 0; + left: calc(50% + (${contentTextWidth}rem / 2) + ${cardContentMargin}rem); + right: unset; top: ${(props: CardProps) => `${props.topOffset || getHighlightBottomOffset(props.container, props.highlight)}px;`} ${(props: CardProps) => !!props.isActive && css` left: calc(50% + (${contentTextWidth}rem / 2) + ${cardFocusedContentMargin}rem); - right: unset; `} `; diff --git a/src/app/messages/en/messages.json b/src/app/messages/en/messages.json index 9b308a1f7d..90b423221d 100644 --- a/src/app/messages/en/messages.json +++ b/src/app/messages/en/messages.json @@ -72,6 +72,8 @@ "i18n:highlighter:highlight:end:search": "End of search result", "i18n:highlighter:highlight:start:search": "Start search result", "i18n:highlighter:highlight:start-selected:search": "Start selected search result", + "i18n:highlighting:create-instructions": "Press Enter or click here to create highlight", + "i18n:highlighting:instructions": "Press Enter or click here to edit highlight", "i18n:highlighting:button:cancel": "Cancel", "i18n:highlighting:button:delete": "Delete", "i18n:highlighting:button:save": "Save", diff --git a/src/app/messages/es/messages.json b/src/app/messages/es/messages.json index 920fb32a20..aba94422b3 100644 --- a/src/app/messages/es/messages.json +++ b/src/app/messages/es/messages.json @@ -69,6 +69,8 @@ "i18n:highlighter:highlight:end:search": "Final de resultado de búsqueda", "i18n:highlighter:highlight:start:search": "Comienzo de resultado de búsqueda", "i18n:highlighter:highlight:start-selected:search": "Comienzo de resultado de búsqueda seleccionado", + "i18n:highlighting:create-instructions": "Presione Enter o haga clic aquí para crear lo resaltado", + "i18n:highlighting:instructions": "Presione Enter o haga clic aquí para editar lo resaltado", "i18n:highlighting:button:cancel": "Cancelar", "i18n:highlighting:button:delete": "Eliminar", "i18n:highlighting:button:save": "Guardar", diff --git a/src/app/messages/pl/messages.json b/src/app/messages/pl/messages.json index ae90c59527..6ac758b623 100644 --- a/src/app/messages/pl/messages.json +++ b/src/app/messages/pl/messages.json @@ -69,6 +69,8 @@ "i18n:highlighter:highlight:end:search": "Koniec zakreślenia wyniku wyszukiwania", "i18n:highlighter:highlight:start:search": "Początek zakreślenia wyniku wyszukiwania", "i18n:highlighter:highlight:start-selected:search": "Początek wybranego wyniku wyszukiwania", + "i18n:highlighting:create-instructions": "Naciśnij Enter lub kliknij tutaj, aby utworzyć wyróżnienie.", + "i18n:highlighting:instructions": "Naciśnij Enter lub kliknij tutaj, aby edytować wyróżnienie", "i18n:highlighting:button:cancel": "Anuluj", "i18n:highlighting:button:delete": "Usuń", "i18n:highlighting:button:save": "Zachowaj",