diff --git a/src/app/components/Modal/__snapshots__/Modal.spec.tsx.snap b/src/app/components/Modal/__snapshots__/Modal.spec.tsx.snap index c18ee8323d..e66707d889 100644 --- a/src/app/components/Modal/__snapshots__/Modal.spec.tsx.snap +++ b/src/app/components/Modal/__snapshots__/Modal.spec.tsx.snap @@ -68,6 +68,7 @@ exports[`Modal matches snapshot 1`] = ` -ms-flex-align: center; align-items: center; margin: 0; + padding: 1.5rem 0; } .c6 { @@ -262,6 +263,7 @@ exports[`Modal matches snapshot with children 1`] = ` -ms-flex-align: center; align-items: center; margin: 0; + padding: 1.5rem 0; } .c6 { diff --git a/src/app/components/Modal/styles.tsx b/src/app/components/Modal/styles.tsx index a9468ccaab..9adfa21447 100644 --- a/src/app/components/Modal/styles.tsx +++ b/src/app/components/Modal/styles.tsx @@ -37,6 +37,7 @@ export const Heading = styled.h1` display: flex; align-items: center; margin: 0; + padding: ${modalPadding * 0.5}rem 0; `; // tslint:disable-next-line:variable-name diff --git a/src/app/content/__snapshots__/routes.spec.tsx.snap b/src/app/content/__snapshots__/routes.spec.tsx.snap index d394eba369..933101c580 100644 --- a/src/app/content/__snapshots__/routes.spec.tsx.snap +++ b/src/app/content/__snapshots__/routes.spec.tsx.snap @@ -286,10 +286,6 @@ Array [ .c4 .highlight { position: relative; z-index: 1; - -webkit-user-select: none; - -moz-user-select: none; - -ms-user-select: none; - user-select: none; } .c4 .MathJax_Display .highlight, diff --git a/src/app/content/components/Page.spec.tsx b/src/app/content/components/Page.spec.tsx index 6fd7f1cc5f..35481274e9 100644 --- a/src/app/content/components/Page.spec.tsx +++ b/src/app/content/components/Page.spec.tsx @@ -1306,7 +1306,8 @@ describe('Page', () => { references: [], })); - await new Promise((resolve) => setTimeout(resolve, 10)); + await Promise.resolve(); + await Promise.resolve(); // deferred scroll execution (scrollToTarget uses setImmediate) await new Promise((resolve) => setImmediate(resolve)); diff --git a/src/app/content/components/Page/PageContent.tsx b/src/app/content/components/Page/PageContent.tsx index fff44152b5..30052fb670 100644 --- a/src/app/content/components/Page/PageContent.tsx +++ b/src/app/content/components/Page/PageContent.tsx @@ -58,7 +58,6 @@ export default styled(MainContent)` .highlight { position: relative; z-index: 1; - user-select: none; } /* stylelint-disable selector-class-pattern */ diff --git a/src/app/content/components/__snapshots__/Content.spec.tsx.snap b/src/app/content/components/__snapshots__/Content.spec.tsx.snap index 41a1a88bf2..7fa7c20754 100644 --- a/src/app/content/components/__snapshots__/Content.spec.tsx.snap +++ b/src/app/content/components/__snapshots__/Content.spec.tsx.snap @@ -711,10 +711,6 @@ Array [ .c74 .highlight { position: relative; z-index: 1; - -webkit-user-select: none; - -moz-user-select: none; - -ms-user-select: none; - user-select: none; } .c74 .MathJax_Display .highlight, @@ -8076,10 +8072,6 @@ Array [ .c74 .highlight { position: relative; z-index: 1; - -webkit-user-select: none; - -moz-user-select: none; - -ms-user-select: none; - user-select: none; } .c74 .MathJax_Display .highlight, diff --git a/src/app/content/highlights/components/Card.tsx b/src/app/content/highlights/components/Card.tsx index ff31d322ea..a737c30135 100644 --- a/src/app/content/highlights/components/Card.tsx +++ b/src/app/content/highlights/components/Card.tsx @@ -170,9 +170,6 @@ function Card(props: CardProps) { ); } -type ComputedProps = ReturnType; -type CommonProps = ComputedProps['commonProps']; - function NoteOrCard({ props, setHighlightRemoved, @@ -182,7 +179,7 @@ function NoteOrCard({ props: CardPropsWithBookAndPage; setHighlightRemoved: React.Dispatch>; locationFilterId: string; - computedProps: ComputedProps; + computedProps: ReturnType; }) { const { focusCard, @@ -222,7 +219,7 @@ function NoteOrCard({ /> ) : ( ; type EditCardProps = { - commonProps: CommonProps & {onRemove: () => void}; + commonProps: object; cardProps: CardPropsWithBookAndPage; locationFilterId: string; } & Pick; diff --git a/src/app/content/highlights/components/CardWrapper.spec.tsx b/src/app/content/highlights/components/CardWrapper.spec.tsx index fafb81044f..6dcd8f94a4 100644 --- a/src/app/content/highlights/components/CardWrapper.spec.tsx +++ b/src/app/content/highlights/components/CardWrapper.spec.tsx @@ -303,11 +303,9 @@ describe('CardWrapper', () => { const highlight2 = createMockHighlight('id2'); const highlightElement1 = document.createElement('span'); const highlightElement2 = document.createElement('span'); - highlight1.elements.push(highlightElement1); - highlight2.elements.push(highlightElement2); container.appendChild(highlightElement1); container.appendChild(highlightElement2); - renderer.create( + const component = renderer.create( { ); - renderer.act(() => { - store.dispatch(focusHighlight(highlight1.id)); - }); - - // These tests get code coverage but do not update the highlight structures - // so that we can see that they worked as expected + const cards = component.root.findAllByType(Card); // Expect cards to be hidden renderer.act(() => { @@ -350,11 +343,8 @@ describe('CardWrapper', () => { }); }); - // Set focusedHighlight, and do double=click - renderer.act(() => { - highlightElement1.dispatchEvent(new Event('focus', { bubbles: true })); - highlightElement1.dispatchEvent(new Event('dblclick', { bubbles: true })); - }); + expect(cards[0].props.isHidden).toBe(false); + expect(cards[1].props.isHidden).toBe(false); }); it( diff --git a/src/app/content/highlights/components/CardWrapper.tsx b/src/app/content/highlights/components/CardWrapper.tsx index dc5b7e1d44..70bb987c60 100644 --- a/src/app/content/highlights/components/CardWrapper.tsx +++ b/src/app/content/highlights/components/CardWrapper.tsx @@ -7,7 +7,7 @@ import styled from 'styled-components'; import { isHtmlElement } from '../../../guards'; import { useFocusLost, useKeyCombination, useFocusHighlight } from '../../../reactUtils'; import { AppState } from '../../../types'; -import { assertDefined, assertDocument } from '../../../utils'; +import { assertDefined } from '../../../utils'; import * as selectSearch from '../../search/selectors'; import * as contentSelect from '../../selectors'; import { highlightKeyCombination } from '../constants'; @@ -25,228 +25,134 @@ export interface WrapperProps { className?: string; } -function checkIfHiddenByCollapsedAncestor(highlight: Highlight) { - const highlightElement = highlight.elements[0] as HTMLElement; - const collapsedAncestor = highlightElement - ? highlightElement.closest('details[data-type="solution"]:not([open])') - : null; - return Boolean(collapsedAncestor); -} - -function useCardPositionObserver( - container: HTMLElement, - focusedHighlight: Highlight | undefined, - highlights: Highlight[], - cardsHeights: Map -) { - const [offsets, setOffsets] = React.useState>(new Map()); +// tslint:disable-next-line:variable-name +const Wrapper = ({highlights, className, container, highlighter}: WrapperProps) => { + const element = React.useRef(null); const [cardsPositions, setCardsPositions] = React.useState>(new Map()); - const getOffsetsForHighlight = React.useCallback((highlight: Highlight) => { - const newOffsets = assertDefined( - getHighlightOffset(container, highlight), - `Couldn't get offsets for highlight with an id: ${highlight.id}` - ); - - setOffsets((state) => new Map(state).set(highlight.id, newOffsets)); - return newOffsets; - }, [container]); - const updatePositions = React.useCallback(() => updateCardsPositions( - focusedHighlight, - highlights, - cardsHeights, - getOffsetsForHighlight, - checkIfHiddenByCollapsedAncestor - ), [cardsHeights, focusedHighlight, getOffsetsForHighlight, highlights]); - // This creates a function that doesn't require dependency updates, for use by - // the resizeObserver effect. A little nicer than using a ref. - const [, dispatchPositions] = React.useReducer( - () => setCardsPositions(updatePositions()), - undefined - ); - - React.useEffect(() => dispatchPositions(), [updatePositions]); - - React.useEffect(() => { - const resizeObserver = new ResizeObserver(dispatchPositions); - resizeObserver.observe(container); - return () => resizeObserver.disconnect(); - }, [container]); - - return [cardsPositions, offsets]; -} - -function useCardsHeights() { const [cardsHeights, setCardsHeights] = React.useState>(new Map()); - const onHeightChange = React.useCallback((id: string, ref: React.RefObject) => { - const height = ref.current ? ref.current.offsetHeight : 0; - if (cardsHeights.get(id) !== height) { - setCardsHeights((previous) => new Map(previous).set(id, height)); - } - }, [cardsHeights]); - - 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, - container: HTMLElement -) { + const [offsets, setOffsets] = React.useState>(new Map()); + const [shouldFocusCard, setShouldFocusCard] = React.useState(false); const focusedId = useSelector(focused); const focusedHighlight = React.useMemo( () => highlights.find((highlight) => highlight.id === focusedId), [focusedId, highlights]); - const [shouldFocusCard, setShouldFocusCard] = React.useState(false); - const document = assertDocument(); - const previousRange = React.useRef(''); - const [dblclickStamp, setDblclickStamp] = React.useState(0); - - React.useEffect(() => { - const handler = () => setDblclickStamp(Date.now()); - - document.addEventListener('dblclick', handler); - return () => document.removeEventListener('dblclick', 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. - 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; - } - }, [focusedHighlight, document, dblclickStamp]); - - // Let Enter go from a highlight to the editor - const editOnEnter = React.useCallback(() => { - if (focusedHighlight) { - setShouldFocusCard(true); - } - }, [focusedHighlight]); + const setNewCardsPositionsRef = React.useRef<() => void>(); + const [isHiddenByEscape, dispatch] = React.useReducer( + editCardVisibilityHandler, + new Map(highlights.map((highlight) => [highlight.id, false])) + ); // This function is triggered by keyboard shortcut defined in useKeyCombination(...) // It moves focus between Card component and highlight in the content. - const moveFocus = React.useCallback(({target}: KeyboardEvent) => { - const activeElement = isHtmlElement(target) ? target : null; - const cardIsFocused = focusedHighlight && element.current?.contains(activeElement); + const moveFocus = React.useCallback((event: KeyboardEvent) => { + const activeElement = isHtmlElement(event.target) ? event.target : null; - if (cardIsFocused) { - focusedHighlight.focus(); + if (!focusedHighlight || !activeElement || !element.current) { + return; } - setShouldFocusCard(!cardIsFocused); - }, [element, focusedHighlight]); - useKeyCombination({key: 'Enter'}, editOnEnter); - 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; -} + if (element.current.contains(activeElement)) { + focusedHighlight.focus(); + } else { + setShouldFocusCard(focusedId !== undefined); + } + }, [focusedHighlight, focusedId]); -function CardsForHighlights({highlights, container, focusedHighlight, shouldFocusCard, highlighter}: { - highlights: Highlight[]; - container: HTMLElement; - focusedHighlight: Highlight | undefined; - shouldFocusCard: boolean; - highlighter: Highlighter; -}) { - const [cardsHeights, onHeightChange] = useCardsHeights(); - const [cardsPositions, offsets] = useCardPositionObserver( - container, - focusedHighlight, - highlights, - cardsHeights - ); - const [isHiddenByEscape, dispatch] = React.useReducer( - editCardVisibilityHandler, - new Map(highlights.map((highlight) => [highlight.id, false])) - ); const hideCard = () => { - focusedHighlight?.focus(); - dispatch({ type: 'HIDE', id: focusedHighlight?.id }); + dispatch({ type: 'HIDE', id: focusedId }); }; + const showCard = (cardId: string | undefined) => { dispatch({ type: 'SHOW', id: cardId }); }; + + useKeyCombination(highlightKeyCombination, moveFocus, noopKeyCombinationHandler([container, element])); + /* * Allow to show EditCard using Enter key * It is important to preserve the default behavior of Enter key */ - useKeyCombination({key: 'Enter'}, () => showCard(focusedHighlight?.id), undefined, false); + useKeyCombination({key: 'Enter'}, () => showCard(focusedId), undefined, false); // Allow to hide EditCard using Escape key useKeyCombination({key: 'Escape'}, hideCard, undefined, false); + + // 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), [setShouldFocusCard])); useFocusHighlight(showCard, highlights); - return <> - {highlights.map((highlight, index) => { - const focusThisCard = shouldFocusCard && focusedHighlight === highlight; - return ) => onHeightChange(highlight.id, ref)} - zIndex={highlights.length - index} - shouldFocusCard={focusThisCard} - isHidden={checkIfHiddenByCollapsedAncestor(highlight) || isHiddenByEscape.get(highlight.id)} - />; - })} - ; -} -// 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 onHeightChange = React.useCallback((id: string, ref: React.RefObject) => { + const height = ref.current && ref.current.offsetHeight; + if (cardsHeights.get(id) !== height) { + setCardsHeights((previous) => new Map(previous).set(id, height === null ? 0 : height)); + } + }, [cardsHeights]); - return
- -
; -}; + const getOffsetsForHighlight = React.useCallback((highlight: Highlight) => { + const newOffsets = assertDefined( + getHighlightOffset(container, highlight), + `Couldn't get offsets for highlight with an id: ${highlight.id}` + ); + setOffsets((state) => new Map(state).set(highlight.id, newOffsets)); + return newOffsets; + }, [container]); -function MaybeWrapper(props: WrapperProps) { - if (!props.highlights.length) { - return null; - } - return ; -} + const checkIfHiddenByCollapsedAncestor = (highlight: Highlight) => { + const highlightElement = highlight.elements[0] as HTMLElement; + const collapsedAncestor = highlightElement + ? highlightElement.closest('details[data-type="solution"]:not([open])') + : null; + return Boolean(collapsedAncestor); + }; + + React.useEffect(() => { + setNewCardsPositionsRef.current = () => { + const positions = updateCardsPositions( + focusedHighlight, + highlights, + cardsHeights, + getOffsetsForHighlight, + checkIfHiddenByCollapsedAncestor + ); + setCardsPositions(positions); + }; + setNewCardsPositionsRef.current(); + }, [cardsHeights, focusedHighlight, getOffsetsForHighlight, highlights]); + + React.useEffect(() => { + const resizeObserver = new ResizeObserver(() => { + assertDefined( + setNewCardsPositionsRef.current, + 'setNewCardsPositionsRef should be already defined by useEffect' + )(); + }); + resizeObserver.observe(container); + return () => { + resizeObserver.disconnect(); + }; + }, [container]); + + return highlights.length + ?
+ {highlights.map((highlight, index) => { + const focusThisCard = shouldFocusCard && focusedId === highlight.id; + return ) => onHeightChange(highlight.id, ref)} + zIndex={highlights.length - index} + shouldFocusCard={focusThisCard} + isHidden={checkIfHiddenByCollapsedAncestor(highlight) || isHiddenByEscape.get(highlight.id)} + />; + })} +
+ : null; +}; export default connect( (state: AppState) => ({ @@ -254,6 +160,6 @@ export default connect( hasQuery: !!selectSearch.query(state), isTocOpen: contentSelect.tocOpen(state), }) -)(styled(MaybeWrapper)` +)(styled(Wrapper)` ${mainWrapperStyles} `); diff --git a/src/app/content/highlights/components/ColorPicker.tsx b/src/app/content/highlights/components/ColorPicker.tsx index 3e7f86d0da..72372e6d1d 100644 --- a/src/app/content/highlights/components/ColorPicker.tsx +++ b/src/app/content/highlights/components/ColorPicker.tsx @@ -174,5 +174,5 @@ export default styled(ColorPicker)` flex-direction: row; overflow: visible; gap: ${cardPadding}rem; - padding: 0 0.3rem ${cardPadding}rem; + padding: ${cardPadding}rem 0.3rem; `; diff --git a/src/app/content/highlights/components/EditCard.spec.tsx b/src/app/content/highlights/components/EditCard.spec.tsx index 7b5f1de451..1a9c806d74 100644 --- a/src/app/content/highlights/components/EditCard.spec.tsx +++ b/src/app/content/highlights/components/EditCard.spec.tsx @@ -57,7 +57,7 @@ describe('EditCard', () => { }; const component = renderer.create( - + ); @@ -70,7 +70,7 @@ describe('EditCard', () => { .mockReturnValue(formatUser(testAccountsUser)); const component = renderer.create( - + ); @@ -89,7 +89,6 @@ describe('EditCard', () => { {...editCardProps} data={highlightData} isActive={true} - shouldFocusCard={true} /> ); @@ -129,7 +128,6 @@ describe('EditCard', () => { {...editCardProps} isActive={true} data={data} - shouldFocusCard={true} /> ); @@ -168,7 +166,7 @@ describe('EditCard', () => { .mockReturnValue(formatUser(testAccountsUser)); const component = renderer.create( - + ); @@ -191,7 +189,6 @@ describe('EditCard', () => { {...editCardProps} data={data} isActive={true} - shouldFocusCard={true} /> ); @@ -257,7 +254,6 @@ describe('EditCard', () => { pageId='pageId' isActive={true} onCreate={jest.fn()} - shouldFocusCard={true} /> ); @@ -487,7 +483,6 @@ describe('EditCard', () => { locationFilterId='locationId' pageId='pageId' isActive={true} - shouldFocusCard={true} /> ); @@ -520,7 +515,7 @@ describe('EditCard', () => { .mockReturnValue(formatUser(testAccountsUser)); const component = renderer.create( - + ); @@ -539,7 +534,7 @@ describe('EditCard', () => { .mockReturnValue(formatUser(testAccountsUser)); const component = renderer.create( - + ); @@ -559,7 +554,7 @@ describe('EditCard', () => { .mockReturnValue(formatUser(testAccountsUser)); const component = renderer.create( - + ); @@ -581,7 +576,7 @@ describe('EditCard', () => { const component = renderer.create( - + ); @@ -606,7 +601,6 @@ describe('EditCard', () => { {...{...editCardProps, hasUnsavedHighlight: false}} onHeightChange={onHeightChange} isActive={true} - shouldFocusCard={true} /> @@ -668,7 +662,6 @@ describe('EditCard', () => { isActive={true} data={highlightData} hasUnsavedHighlight={true} - shouldFocusCard={true} /> ); @@ -703,7 +696,6 @@ describe('EditCard', () => { {...editCardProps} isActive={true} data={undefined} - shouldFocusCard={true} /> ); @@ -768,23 +760,6 @@ describe('EditCard', () => { expect(spyAnalytics).not.toHaveBeenCalled(); }); - it('shows login for unauthenticated user if the card is active', () => { - renderer.create( - - - - ); - - // Wait for React.useEffect - renderer.act(() => undefined); - - }); - it('call onHeightChange when element mounts', () => { const onClickOutside = jest.spyOn(onClickOutsideModule, 'default'); onClickOutside.mockReturnValue(() => () => null); @@ -800,7 +775,6 @@ describe('EditCard', () => { {...editCardProps} isActive={true} onHeightChange={onHeightChange} - shouldFocusCard={true} /> , {createNodeMock} diff --git a/src/app/content/highlights/components/EditCard.tsx b/src/app/content/highlights/components/EditCard.tsx index 729df9738d..ad071cf952 100644 --- a/src/app/content/highlights/components/EditCard.tsx +++ b/src/app/content/highlights/components/EditCard.tsx @@ -46,7 +46,6 @@ export interface EditCardProps { data?: HighlightData; className: string; shouldFocusCard: boolean; - minimize?: boolean; } // tslint:disable-next-line:variable-name @@ -74,20 +73,18 @@ function LoginOrEdit({ role='dialog' aria-label={formatMessage({id: 'i18n:highlighter:edit-note:label'})} > - { - authenticated ? ( - (props.shouldFocusCard || props.data?.annotation) ? ( -
- - - ) : - Press Enter or double-click highlight to edit highlight - ) : - } +
+ {authenticated ? ( + + ) : ( + + )} + ); } @@ -457,22 +454,24 @@ function useOnColorChange(props: EditCardProps) { } function useSaveAnnotation( - { data, pageId, locationFilterId, highlight, onCancel }: EditCardProps, + props: EditCardProps, element: React.RefObject, pendingAnnotation: string ) { const dispatch = useDispatch(); const trackEditAnnotation = useAnalyticsEvent('editAnnotation'); + const { pageId, locationFilterId, highlight } = props; + const onCancel = props.onCancel; return React.useCallback( (toSave: HighlightData) => { - const definedData = assertDefined( - data, + const data = assertDefined( + props.data, 'Can\'t update highlight that doesn\'t exist' ); - const addedNote = definedData.annotation === undefined; - const { updatePayload, preUpdateData } = generateUpdatePayload(definedData, { + const addedNote = data.annotation === undefined; + const { updatePayload, preUpdateData } = generateUpdatePayload(data, { id: toSave.id, annotation: pendingAnnotation, }); @@ -491,7 +490,6 @@ function useSaveAnnotation( highlight.focus(); }, [ - data, dispatch, element, highlight, @@ -499,6 +497,7 @@ function useSaveAnnotation( onCancel, pageId, pendingAnnotation, + props.data, trackEditAnnotation, ] ); @@ -509,6 +508,7 @@ export default styled(EditCard)` background: ${theme.color.neutral.formBackground}; user-select: none; overflow: visible; + padding-top: 0 !important; ${ButtonGroup} { margin-top: ${cardPadding}rem; diff --git a/src/app/content/highlights/components/HighlightsPopUp.tsx b/src/app/content/highlights/components/HighlightsPopUp.tsx index ead84fafc2..1a70cd4805 100644 --- a/src/app/content/highlights/components/HighlightsPopUp.tsx +++ b/src/app/content/highlights/components/HighlightsPopUp.tsx @@ -94,63 +94,59 @@ interface Props { loggedOut: boolean; } -function useCloseAndTrack(closeFn: () => void) { - const trackOpenCloseMH = useAnalyticsEvent('openCloseMH'); - - return React.useCallback((method: string) => () => { - closeFn(); - trackOpenCloseMH(method); - }, [closeFn, trackOpenCloseMH]); -} - // tslint:disable-next-line: variable-name -const HighlightsPopUp = ({ closeMyHighlights, ...props }: Omit) => { +const HighlightsPopUp = ({ closeMyHighlights, ...props }: Props) => { const popUpRef = React.useRef(null); const intl = useIntl(); - const closeAndTrack = useCloseAndTrack(closeMyHighlights); + const trackOpenCloseMH = useAnalyticsEvent('openCloseMH'); - useOnEsc(true, closeAndTrack('esc')); + const closeAndTrack = React.useCallback((method: string) => () => { + closeMyHighlights(); + trackOpenCloseMH(method); + }, [closeMyHighlights, trackOpenCloseMH]); - React.useEffect(() => popUpRef.current?.focus(), []); + useOnEsc(props.myHighlightsOpen, closeAndTrack('esc')); - return -
-

- - {(msg) => msg} - -

- - - -
- {props.user ? : } - - ; -}; + React.useEffect(() => { + const popUp = popUpRef.current; -function MaybeHighlightsPopUp({myHighlightsOpen, ...props}: Props) { - if (!myHighlightsOpen) { - return null; - } + if (popUp && props.myHighlightsOpen) { + popUp.focus(); + } + }, [props.myHighlightsOpen]); - return ; -} + return props.myHighlightsOpen ? + +
+

+ + {(msg) => msg} + +

+ + + +
+ {props.user ? : } + + + : null; +}; export default connect( (state: AppState) => ({ @@ -162,4 +158,4 @@ export default connect( (dispatch: Dispatch) => ({ closeMyHighlights: () => dispatch(closeMyHighlightsAction()), }) -)(MaybeHighlightsPopUp); +)(HighlightsPopUp); diff --git a/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap b/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap index 249f67d840..2e24c3b2f6 100644 --- a/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap +++ b/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap @@ -449,7 +449,7 @@ exports[`ContextMenu match snapshot when open 1`] = ` flex-direction: row; overflow: visible; gap: 0.8rem; - padding: 0 0.3rem 0.8rem; + padding: 0.8rem 0.3rem; } .c4 { diff --git a/src/app/content/highlights/components/__snapshots__/ColorPicker.spec.tsx.snap b/src/app/content/highlights/components/__snapshots__/ColorPicker.spec.tsx.snap index 7170e4dfa9..703350f97e 100644 --- a/src/app/content/highlights/components/__snapshots__/ColorPicker.spec.tsx.snap +++ b/src/app/content/highlights/components/__snapshots__/ColorPicker.spec.tsx.snap @@ -320,7 +320,7 @@ exports[`ColorPicker matches snapshot no selection 1`] = ` flex-direction: row; overflow: visible; gap: 0.8rem; - padding: 0 0.3rem 0.8rem; + padding: 0.8rem 0.3rem; }
-
+
+ mock-confirmation="true" + /> +
`; @@ -207,6 +214,7 @@ exports[`EditCard matches snapshot with data 1`] = ` -ms-user-select: none; user-select: none; overflow: visible; + padding-top: 0 !important; } .c0 .c1 { @@ -262,6 +270,7 @@ exports[`EditCard matches snapshot without data 1`] = ` -ms-user-select: none; user-select: none; overflow: visible; + padding-top: 0 !important; } .c0 .c1 { @@ -273,19 +282,24 @@ exports[`EditCard matches snapshot without data 1`] = ` className="c0" role="dialog" > -
+
+ mock-confirmation="true" + /> +
`; diff --git a/src/app/errors/components/__snapshots__/ErrorModal.spec.tsx.snap b/src/app/errors/components/__snapshots__/ErrorModal.spec.tsx.snap index 39fb8e0785..b69a359458 100644 --- a/src/app/errors/components/__snapshots__/ErrorModal.spec.tsx.snap +++ b/src/app/errors/components/__snapshots__/ErrorModal.spec.tsx.snap @@ -68,6 +68,7 @@ exports[`ErrorModal matches snapshot 1`] = ` -ms-flex-align: center; align-items: center; margin: 0; + padding: 1.5rem 0; } .c7 { @@ -390,6 +391,7 @@ exports[`ErrorModal matches snapshots with recorded error ids 1`] = ` -ms-flex-align: center; align-items: center; margin: 0; + padding: 1.5rem 0; } .c7 { diff --git a/src/app/reactUtils.ts b/src/app/reactUtils.ts index ce6d7de777..017c8fdf91 100644 --- a/src/app/reactUtils.ts +++ b/src/app/reactUtils.ts @@ -142,11 +142,13 @@ export const onFocusInOrOutHandler = ( }; export const useFocusLost = (ref: React.RefObject, isEnabled: boolean, cb: () => void) => { - React.useEffect(() => onFocusInOrOutHandler(ref, isEnabled, cb, 'focusout')(), [ref, isEnabled, cb]); + // eslint-disable-next-line react-hooks/exhaustive-deps + React.useEffect(onFocusInOrOutHandler(ref, isEnabled, cb, 'focusout'), [ref, isEnabled, cb]); }; export const useFocusIn = (ref: React.RefObject, isEnabled: boolean, cb: () => void) => { - React.useEffect(() => onFocusInOrOutHandler(ref, isEnabled, cb, 'focusin')(), [ref, isEnabled, cb]); + // eslint-disable-next-line react-hooks/exhaustive-deps + React.useEffect(onFocusInOrOutHandler(ref, isEnabled, cb, 'focusin'), [ref, isEnabled, cb]); }; export const onDOMEventHandler = ( @@ -182,21 +184,23 @@ export const useTimeout = (delay: number, callback: () => void) => { const timeout = React.useRef(); const timeoutHandler = () => savedCallback.current && savedCallback.current(); - const reset = React.useCallback(() => { + const reset = () => { if (timeout.current) { clearTimeout(timeout.current); } timeout.current = setTimeout(timeoutHandler, delay); - }, [delay]); + }; React.useEffect(() => { savedCallback.current = callback; + // eslint-disable-next-line react-hooks/exhaustive-deps }, [callback]); React.useEffect(() => { reset(); - }, [reset]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [delay]); React.useEffect(() => () => clearTimeout(assertDefined(timeout.current, 'timeout ID can\'t be undefined')), []); @@ -256,7 +260,8 @@ export const useOnKey = ( isEnabled: boolean, cb: () => void ) => { - React.useEffect(() => onKeyHandler(config, element, isEnabled, cb)(), [config, element, isEnabled, cb]); + // eslint-disable-next-line react-hooks/exhaustive-deps + React.useEffect(onKeyHandler(config, element, isEnabled, cb), [config, element, isEnabled, cb]); }; export const onEscCallbacks: Array<() => void> = []; @@ -458,7 +463,8 @@ export const useKeyCombination = ( if (preventDefault) event.preventDefault(); callback(event); } - }, [callback, noopHandler, options, preventDefault]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [callback, options]); React.useEffect(() => { document.addEventListener('keydown', handler); @@ -486,15 +492,16 @@ export const useFocusHighlight = (showCard: (id: string) => void, highlights: Hi When clicking on a highlight, the target is a mark element and we need to find the first span inside it to get the highlight as expected */ - target = event.target.querySelector('span'); + target = event.target.querySelectorAll('span')[0]; } } else { target = event.target; } const highlight = highlights.find(h => - h.elements && (h.elements as Element[]).some(el => + h.elements && h.elements.some(el => el === target || - (!!el && typeof el.contains === 'function' && el.contains(target as Element)) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (!!el && typeof (el as any).contains === 'function' && (el as any).contains(target)) ) );