diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/current-file-container.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/current-file-container.tsx index ccbc1f8004..446b0dc83f 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/current-file-container.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/current-file-container.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo } from 'react' +import { useMemo } from 'react' import Container from './container' import Toolbar from './toolbar/toolbar' import Nav from './nav' @@ -9,10 +9,7 @@ import CommentEntry from './entries/comment-entry' import AddCommentEntry from './entries/add-comment-entry' import BulkActionsEntry from './entries/bulk-actions-entry/bulk-actions-entry' import PositionedEntries from './positioned-entries' -import { - useReviewPanelUpdaterFnsContext, - useReviewPanelValueContext, -} from '../../context/review-panel/review-panel-context' +import { useReviewPanelValueContext } from '../../context/review-panel/review-panel-context' import useCodeMirrorContentHeight from '../../hooks/use-codemirror-content-height' import { ReviewPanelEntry } from '../../../../../../types/review-panel/entry' import { @@ -35,7 +32,6 @@ function CurrentFileContainer() { entryHover, nVisibleSelectedChanges: nChanges, } = useReviewPanelValueContext() - const { setEntryHover, toggleReviewPanel } = useReviewPanelUpdaterFnsContext() const contentHeight = useCodeMirrorContentHeight() const currentDocEntries = @@ -47,14 +43,6 @@ function CurrentFileContainer() { > }, [currentDocEntries]) - const onMouseEnter = useCallback(() => { - setEntryHover(true) - }, [setEntryHover]) - - const onMouseLeave = useCallback(() => { - setEntryHover(false) - }, [setEntryHover]) - return ( ) } @@ -121,9 +106,6 @@ function CurrentFileContainer() { focused={entry.focused} entryIds={entry.entry_ids} timestamp={entry.metadata.ts} - onMouseEnter={onMouseEnter} - onMouseLeave={onMouseLeave} - onIndicatorClick={toggleReviewPanel} /> ) } @@ -143,9 +125,6 @@ function CurrentFileContainer() { offset={entry.offset} focused={entry.focused} permissions={permissions} - onMouseEnter={onMouseEnter} - onMouseLeave={onMouseLeave} - onIndicatorClick={toggleReviewPanel} /> ) } diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/aggregate-change-entry.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/aggregate-change-entry.tsx index ef33626a3d..cc5bd89a9f 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/aggregate-change-entry.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/aggregate-change-entry.tsx @@ -9,6 +9,8 @@ import { formatTime } from '../../../../utils/format-date' import classnames from 'classnames' import comparePropsWithShallowArrayCompare from '../utils/compare-props-with-shallow-array-compare' import { BaseChangeEntryProps } from '../types/base-change-entry-props' +import useIndicatorHover from '../hooks/use-indicator-hover' +import EntryIndicator from './entry-indicator' interface AggregateChangeEntryProps extends BaseChangeEntryProps { replacedContent: string @@ -26,15 +28,19 @@ function AggregateChangeEntry({ entryIds, timestamp, contentLimit = 17, - onMouseEnter, - onMouseLeave, - onIndicatorClick, }: AggregateChangeEntryProps) { const { t } = useTranslation() const { acceptChanges, rejectChanges, gotoEntry, handleLayoutChange } = useReviewPanelUpdaterFnsContext() const [isDeletionCollapsed, setIsDeletionCollapsed] = useState(true) const [isInsertionCollapsed, setIsInsertionCollapsed] = useState(true) + const { + hoverCoords, + indicatorRef, + handleEntryMouseLeave, + handleIndicatorMouseEnter, + handleIndicatorClick, + } = useIndicatorHover() const deletionNeedsCollapsing = replacedContent.length > contentLimit const insertionNeedsCollapsing = content.length > contentLimit @@ -76,20 +82,19 @@ function AggregateChangeEntry({ return ( - {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */} -
-
+
- {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} -
{isInsert ? : } -
+
void - onMouseLeave?: () => void - onIndicatorClick?: () => void } & Pick function CommentEntry({ @@ -37,9 +36,6 @@ function CommentEntry({ offset, focused, permissions, - onMouseEnter, - onMouseLeave, - onIndicatorClick, }: CommentEntryProps) { const { t } = useTranslation() const { gotoEntry, resolveComment, submitReply, handleLayoutChange } = @@ -48,6 +44,13 @@ function CommentEntry({ const [animating, setAnimating] = useState(false) const [resolved, setResolved] = useState(false) const entryDivRef = useRef(null) + const { + hoverCoords, + indicatorRef, + handleEntryMouseLeave, + handleIndicatorMouseEnter, + handleIndicatorClick, + } = useIndicatorHover() const handleEntryClick = (e: React.MouseEvent) => { const target = e.target as Element @@ -120,9 +123,9 @@ function CommentEntry({ return ( {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
- {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} -
-
+
) { - return ( +}: React.ComponentProps<'div'> & { + hoverCoords?: Coordinates | null +}) { + const container = (
) + + if (hoverCoords) { + // Render in a floating positioned container + return createPortal( +
+ {container} +
, + document.body + ) + } else { + return container + } } export default EntryContainer diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-indicator.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-indicator.tsx new file mode 100644 index 0000000000..334a755bde --- /dev/null +++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-indicator.tsx @@ -0,0 +1,30 @@ +import classnames from 'classnames' +import { forwardRef } from 'react' + +type EntryIndicatorProps = { + focused: boolean + onMouseEnter: () => void + onClick: () => void + children: React.ReactNode +} + +const EntryIndicator = forwardRef( + ({ focused, onMouseEnter, onClick, children }, ref) => { + return ( + /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */ +
+ {children} +
+ ) + } +) +EntryIndicator.displayName = 'EntryIndicator' + +export default EntryIndicator diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts b/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts new file mode 100644 index 0000000000..c1517ee706 --- /dev/null +++ b/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts @@ -0,0 +1,60 @@ +import { useRef, useState } from 'react' +import { flushSync } from 'react-dom' +import { useReviewPanelUpdaterFnsContext } from '../../../context/review-panel/review-panel-context' +import { useLayoutContext } from '../../../../../shared/context/layout-context' +import useScopeValue from '../../../../../shared/hooks/use-scope-value' + +export type Coordinates = { + x: number + y: number +} + +function useIndicatorHover() { + const [hoverCoords, setHoverCoords] = useState(null) + const [layoutToLeft] = useScopeValue('reviewPanel.layoutToLeft') + const { toggleReviewPanel } = useReviewPanelUpdaterFnsContext() + const { reviewPanelOpen } = useLayoutContext() + const { setLayoutSuspended, handleLayoutChange } = + useReviewPanelUpdaterFnsContext() + const indicatorRef = useRef(null) + + const handleEntryMouseLeave = () => { + if (!reviewPanelOpen && !layoutToLeft) { + // Use flushSync to ensure that React renders immediately. This is + // necessary to ensure that the subsequent layout update acts on the + // updated DOM. + flushSync(() => { + setHoverCoords(null) + setLayoutSuspended(false) + }) + handleLayoutChange(true) + } + } + + const handleIndicatorMouseEnter = () => { + if (!layoutToLeft) { + const rect = indicatorRef.current?.getBoundingClientRect() + setHoverCoords({ + x: rect?.left || 0, + y: rect?.top || 0, + }) + setLayoutSuspended(true) + } + } + + const handleIndicatorClick = () => { + setHoverCoords(null) + setLayoutSuspended(false) + toggleReviewPanel() + } + + return { + hoverCoords, + indicatorRef, + handleEntryMouseLeave, + handleIndicatorMouseEnter, + handleIndicatorClick, + } +} + +export default useIndicatorHover diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx index 7095984f38..778ac9ae01 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx @@ -24,32 +24,18 @@ type EntryView = { callout: HTMLElement layout: HTMLElement height: number - previousEntryTop: number | null - previousCalloutTop: number | null entry: ReviewPanelEntry visible: boolean positions?: Positions + previousPositions?: Positions } type EntryPositions = Pick -type LayoutInfo = { - focusedEntryIndex: number - overflowTop: number - height: number - positions: EntryPositions[] -} - function css(el: HTMLElement, props: React.CSSProperties) { Object.assign(el.style, props) } -function applyEntryVisibility(entryView: EntryView) { - const visible = !!entryView.entry.screenPos - entryView.wrapper.classList.toggle('rp-entry-hidden', !visible) - return visible -} - function calculateCalloutPosition( screenPos: ReviewPanelEntryScreenPos, entryTop: number, @@ -78,8 +64,8 @@ const calculateEntryViewPositions = ( calculateTop: (originalTop: number, height: number) => number ) => { for (const entryView of entryViews) { - const entryVisible = applyEntryVisibility(entryView) - if (entryVisible) { + entryView.wrapper.classList.toggle('rp-entry-hidden', !entryView.visible) + if (entryView.visible) { const entryTop = calculateTop( entryView.entry.screenPos.y, entryView.height @@ -104,21 +90,24 @@ type PositionedEntriesProps = { children: React.ReactNode } +const initialLayoutInfo = { + focusedEntryIndex: 0, + overflowTop: 0, + height: 0, + positions: [] as EntryPositions[], +} + function PositionedEntries({ entries, contentHeight, children, }: PositionedEntriesProps) { - const { navHeight, toolbarHeight, lineHeight } = useReviewPanelValueContext() + const { navHeight, toolbarHeight, lineHeight, layoutSuspended } = + useReviewPanelValueContext() const containerRef = useRef(null) const { reviewPanelOpen } = useLayoutContext() const animationTimerRef = useRef(null) - const previousLayoutInfoRef = useRef({ - focusedEntryIndex: 0, - overflowTop: 0, - height: 0, - positions: [], - }) + const previousLayoutInfoRef = useRef(initialLayoutInfo) const layout = () => { const container = containerRef.current @@ -158,8 +147,10 @@ function PositionedEntries({ const layoutElement = reviewPanelOpen ? box : indicator if (box && callout && layoutElement) { - const previousEntryTopData = box.dataset.previousEntryTop - const previousCalloutTopData = callout.dataset.previousEntryTop + const previousPositions = previousLayoutInfoRef.current?.positions.find( + pos => pos.entryId === entryId + )?.positions + const visible = Boolean(entry.screenPos) entryViews.push({ entryId, wrapper, @@ -167,17 +158,10 @@ function PositionedEntries({ box, callout, layout: layoutElement, - visible: !!entry.screenPos, + visible, height: 0, - previousEntryTop: - previousEntryTopData !== undefined - ? parseInt(previousEntryTopData) - : null, - previousCalloutTop: - previousCalloutTopData !== undefined - ? parseInt(previousCalloutTopData) - : null, entry, + previousPositions, }) } else { debugConsole.log( @@ -305,10 +289,9 @@ function PositionedEntries({ // Position the main wrapper in its original position, if it had // one, or its new position otherwise - const entryTopInitial = - entryView.previousEntryTop === null - ? entryTop - : entryView.previousEntryTop + const entryTopInitial = entryView.previousPositions + ? entryView.previousPositions.entryTop + : entryTop css(entryView.box, { top: entryTopInitial + overflowTop + 'px', @@ -322,25 +305,20 @@ function PositionedEntries({ entryView.indicator.style.top = entryTopInitial + overflowTop + 'px' } - entryView.box.dataset.previousEntryTop = entryTopInitial + '' - // Position the callout element in its original position, if it had // one, or its new position otherwise calloutEl.classList.toggle( 'rp-entry-callout-inverted', callout.inverted ) - const calloutTopInitial = - entryView.previousCalloutTop === null - ? callout.top - : entryView.previousCalloutTop + const calloutTopInitial = entryView.previousPositions + ? entryView.previousPositions.callout.top + : callout.top css(calloutEl, { top: calloutTopInitial + overflowTop + 'px', height: callout.height + 'px', }) - - entryView.box.dataset.previousCalloutTop = calloutTopInitial + '' } } } @@ -355,19 +333,17 @@ function PositionedEntries({ const { entryTop, callout } = positions // Position the main wrapper, if it's moved - if (entryView.previousEntryTop !== entryTop) { + if (entryView.previousPositions?.entryTop !== entryTop) { entryView.box.style.top = entryTop + overflowTop + 'px' } - entryView.box.dataset.previousEntryTop = entryTop + '' if (entryView.indicator) { entryView.indicator.style.top = entryTop + overflowTop + 'px' } // Position the callout element - if (entryView.previousCalloutTop !== callout.top) { + if (entryView.previousPositions?.callout.top !== callout.top) { calloutEl.style.top = callout.top + overflowTop + 'px' - entryView.callout.dataset.previousCalloutTop = callout.top + '' } } } @@ -430,8 +406,14 @@ function PositionedEntries({ } } - useEventListener('review-panel:layout', () => { - layout() + useEventListener('review-panel:layout', (event: CustomEvent) => { + if (!layoutSuspended) { + // Clear previous positions if forcing a layout + if (event.detail.force) { + previousLayoutInfoRef.current = initialLayoutInfo + } + layout() + } }) // Layout on first render. This is necessary to ensure layout happens when @@ -440,6 +422,11 @@ function PositionedEntries({ dispatchReviewPanelLayout() }, []) + // Ensure layout is performed after opening or closing the review panel + useEffect(() => { + previousLayoutInfoRef.current = initialLayoutInfo + }, [reviewPanelOpen]) + return (
void - onMouseLeave?: () => void - onIndicatorClick?: () => void } diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts index d1a96ac390..bdcba2f30d 100644 --- a/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts +++ b/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts @@ -137,6 +137,7 @@ function useAngularReviewPanelState(): ReviewPanelState { const [isAddingComment, setIsAddingComment] = useState(false) const [navHeight, setNavHeight] = useState(0) const [toolbarHeight, setToolbarHeight] = useState(0) + const [layoutSuspended, setLayoutSuspended] = useState(false) const values = useMemo( () => ({ @@ -164,6 +165,7 @@ function useAngularReviewPanelState(): ReviewPanelState { trackChangesOnForGuests, trackChangesForGuestsAvailable, formattedProjectMembers, + layoutSuspended, }), [ collapsed, @@ -190,6 +192,7 @@ function useAngularReviewPanelState(): ReviewPanelState { trackChangesOnForGuests, trackChangesForGuestsAvailable, formattedProjectMembers, + layoutSuspended, ] ) @@ -220,6 +223,7 @@ function useAngularReviewPanelState(): ReviewPanelState { setIsAddingComment, setNavHeight, setToolbarHeight, + setLayoutSuspended, }), [ handleSetSubview, @@ -246,6 +250,7 @@ function useAngularReviewPanelState(): ReviewPanelState { setIsAddingComment, setNavHeight, setToolbarHeight, + setLayoutSuspended, ] ) diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts index ca60e019bf..9917fd9e79 100644 --- a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts +++ b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts @@ -45,10 +45,11 @@ export interface ReviewPanelState { name: string } > + layoutSuspended: boolean } updaterFns: { handleSetSubview: (subView: SubView) => void - handleLayoutChange: () => void + handleLayoutChange: (force?: boolean) => void gotoEntry: (docId: DocId, entryOffset: number) => void resolveComment: (docId: DocId, entryId: ThreadId) => void deleteComment: (threadId: ThreadId, commentId: CommentId) => void @@ -82,6 +83,9 @@ export interface ReviewPanelState { setToolbarHeight: React.Dispatch< React.SetStateAction> > + setLayoutSuspended: React.Dispatch< + React.SetStateAction> + > } } /* eslint-enable no-use-before-define */ diff --git a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts index 173b14cdd7..f261060312 100644 --- a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts +++ b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts @@ -31,8 +31,10 @@ export const dispatchEditorEvent = (type: string, payload?: unknown) => { }, 0) } -export const dispatchReviewPanelLayout = () => { - window.dispatchEvent(new CustomEvent('review-panel:layout')) +export const dispatchReviewPanelLayout = (force = false) => { + window.dispatchEvent( + new CustomEvent('review-panel:layout', { detail: { force } }) + ) } const scheduleDispatchReviewPanelLayout = debounce( diff --git a/services/web/frontend/stylesheets/app/editor/review-panel.less b/services/web/frontend/stylesheets/app/editor/review-panel.less index 67087429ac..7f51c08f71 100644 --- a/services/web/frontend/stylesheets/app/editor/review-panel.less +++ b/services/web/frontend/stylesheets/app/editor/review-panel.less @@ -232,10 +232,17 @@ .rp-entry-indicator { display: none; z-index: 2; // above .review-panel-toggler + .rp-size-mini & { display: block; } + .rp-floating-entry & { + display: block; + position: static; + width: @review-off-width - 4px; + } + .rp-size-mini &-add-comment { display: none; } @@ -280,8 +287,8 @@ width: @review-panel-width; } - .rp-state-current-file-mini & { - display: none; + .rp-state-current-file-mini &, + .rp-floating-entry & { left: @review-off-width + @rp-entry-arrow-width; box-shadow: 0 0 10px 5px rgba(0, 0, 0, 0.2); z-index: 1; @@ -309,6 +316,17 @@ } } + .rp-state-current-file-mini & { + display: none; + } + + .rp-floating-entry & { + position: absolute; + width: @review-panel-width; + left: @review-off-width + @rp-entry-arrow-width; + top: 0; + } + .rp-state-current-file-mini.rp-layout-left & { left: auto; right: @review-off-width + @rp-entry-arrow-width; @@ -1258,7 +1276,6 @@ button when (@is-overleaf-light = true) { .rp-entry-list-react { position: relative; - overflow-x: hidden; } .rp-state-current-file & { @@ -1299,7 +1316,16 @@ button when (@is-overleaf-light = true) { height: 0; transition: height 150ms; } +} +.rp-floating-entry { + position: absolute; + font-size: @rp-base-font-size; + color: @rp-type-blue; +} + +.ol-cm-review-panel, +.rp-floating-entry { .rp-entry-metadata { button { padding: 0;