From e61eb1b2204ef090e83bc04d2dcbca56dd8900b1 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Thu, 22 Aug 2024 10:44:22 +0100 Subject: [PATCH] Merge pull request #20008 from overleaf/ae-review-panel-empty-state Improve calculations of empty state, mini state and sizes variables in review panel GitOrigin-RevId: 41bcb3b67c9f0019c11b4de0e4590b0407e04e66 --- .../components/review-panel-add-comment.tsx | 3 +- .../components/review-panel-container.tsx | 19 +- .../components/review-panel-current-file.tsx | 202 +++++++++--------- .../components/review-panel-header.tsx | 7 +- .../components/review-panel-overview.tsx | 12 +- .../components/review-panel.tsx | 61 ++---- .../hooks/use-review-panel-styles.ts | 54 +++++ .../utils/has-active-range.ts | 25 +++ .../review-panel-new/utils/is-in-viewport.tsx | 7 - .../review-panel-new/utils/position-items.ts | 48 +++-- .../app/editor/review-panel-new.less | 148 ++++++++----- 11 files changed, 341 insertions(+), 245 deletions(-) create mode 100644 services/web/frontend/js/features/review-panel-new/hooks/use-review-panel-styles.ts create mode 100644 services/web/frontend/js/features/review-panel-new/utils/has-active-range.ts delete mode 100644 services/web/frontend/js/features/review-panel-new/utils/is-in-viewport.tsx diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-add-comment.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-add-comment.tsx index 583c545f27..1b3e504507 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-add-comment.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-add-comment.tsx @@ -4,7 +4,6 @@ import { useCodeMirrorViewContext, } from '@/features/source-editor/components/codemirror-editor' import { EditorSelection } from '@codemirror/state' -import { PANEL_WIDTH } from './review-panel' import { useTranslation } from 'react-i18next' import { useThreadsActionsContext } from '../context/threads-context' @@ -49,7 +48,7 @@ export const ReviewPanelAddComment: FC = () => { return (
{/* eslint-disable-next-line jsx-a11y/no-autofocus */} diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-container.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-container.tsx index 123c5f1a67..c795b5706e 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-container.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-container.tsx @@ -4,19 +4,30 @@ import { memo } from 'react' import ReviewPanel from './review-panel' import { useLayoutContext } from '@/shared/context/layout-context' import { useRangesContext } from '../context/ranges-context' +import { useThreadsContext } from '@/features/review-panel-new/context/threads-context' +import { hasActiveRange } from '@/features/review-panel-new/utils/has-active-range' function ReviewPanelContainer() { const view = useCodeMirrorViewContext() const ranges = useRangesContext() + const threads = useThreadsContext() const { reviewPanelOpen } = useLayoutContext() - const mini = !reviewPanelOpen && !!ranges?.total - - if (!view || (!reviewPanelOpen && !mini)) { + if (!view) { return null } - return ReactDOM.createPortal(, view.scrollDOM) + // the full-width review panel + if (reviewPanelOpen) { + return ReactDOM.createPortal(, view.scrollDOM) + } + + // the mini review panel + if (hasActiveRange(ranges, threads)) { + return ReactDOM.createPortal(, view.scrollDOM) + } + + return null } export default memo(ReviewPanelContainer) diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-current-file.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-current-file.tsx index ae2fa7fba4..ebb95cabdc 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-current-file.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-current-file.tsx @@ -16,7 +16,6 @@ import { DeleteOperation, EditOperation, } from '../../../../../types/change' -import { editorVerticalTopPadding } from '@/features/source-editor/extensions/vertical-overflow' import { useCodeMirrorStateContext, useCodeMirrorViewContext, @@ -27,18 +26,14 @@ import { isDeleteChange, isInsertChange } from '@/utils/operations' import Icon from '@/shared/components/icon' import { positionItems } from '../utils/position-items' import { canAggregate } from '../utils/can-aggregate' -import { isInViewport } from '../utils/is-in-viewport' import ReviewPanelEmptyState from './review-panel-empty-state' import useEventListener from '@/shared/hooks/use-event-listener' +import { hasActiveRange } from '@/features/review-panel-new/utils/has-active-range' -type Positions = Map -type Aggregates = Map> - -type RangesWithPositions = { +type AggregatedRanges = { changes: Change[] comments: Change[] - positions: Positions - aggregates: Aggregates + aggregates: Map> } const ReviewPanelCurrentFile: FC = () => { @@ -47,24 +42,7 @@ const ReviewPanelCurrentFile: FC = () => { const threads = useThreadsContext() const state = useCodeMirrorStateContext() - const [rangesWithPositions, setRangesWithPositions] = - useState() - - const contentRect = view.contentDOM.getBoundingClientRect() - - const editorPaddingTop = editorVerticalTopPadding(view) - const topDiff = contentRect.top - editorPaddingTop - const docLength = state.doc.length - - const screenPosition = useCallback( - (change: Change): number | undefined => { - const pos = Math.min(change.op.p, docLength) - const coords = view.coordsAtPos(pos) - - return coords ? Math.round(coords.top - topDiff) : undefined - }, - [docLength, topDiff, view] - ) + const [aggregatedRanges, setAggregatedRanges] = useState() const selectionCoords = useMemo( () => @@ -85,7 +63,7 @@ const ReviewPanelCurrentFile: FC = () => { ) if (extents) { - previousFocusedItem.current = extents.focusedItemIndex + previousFocusedItem.current = extents.activeItemIndex } } }, []) @@ -124,62 +102,84 @@ const ReviewPanelCurrentFile: FC = () => { } }, [updatePositions]) - const buildEntries = useCallback(() => { + const buildAggregatedRanges = useCallback(() => { if (ranges) { + const output: AggregatedRanges = { + aggregates: new Map(), + changes: [], + comments: [], + } + + let precedingChange: Change | null = null + + for (const change of ranges.changes) { + if ( + precedingChange && + isInsertChange(precedingChange) && + isDeleteChange(change) && + canAggregate(change, precedingChange) + ) { + output.aggregates.set(precedingChange.id, change) + } else { + output.changes.push(change) + } + + precedingChange = change + } + + if (threads) { + for (const comment of ranges.comments) { + if (!threads[comment.op.t]?.resolved) { + output.comments.push(comment) + } + } + } + + setAggregatedRanges(output) + } + }, [threads, ranges]) + + useEffect(() => { + buildAggregatedRanges() + }, [buildAggregatedRanges]) + + useEventListener('editor:viewport-changed', buildAggregatedRanges) + + const [positions, setPositions] = useState>(new Map()) + + const positionsRef = useRef>(new Map()) + + useEffect(() => { + if (aggregatedRanges) { view.requestMeasure({ key: 'review-panel-position', - read(view): RangesWithPositions { - const isVisible = isInViewport(view) + read(view) { + const contentRect = view.contentDOM.getBoundingClientRect() + const docLength = view.state.doc.length - const output: RangesWithPositions = { - positions: new Map(), - aggregates: new Map(), - changes: [], - comments: [], + const screenPosition = (change: Change): number | undefined => { + const pos = Math.min(change.op.p, docLength) // TODO: needed? + const coords = view.coordsAtPos(pos) + + return coords ? Math.round(coords.top - contentRect.top) : undefined } - let precedingChange: Change | null = null - - for (const change of ranges.changes) { - if (isVisible(change)) { - if ( - precedingChange && - isInsertChange(precedingChange) && - isDeleteChange(change) && - canAggregate(change, precedingChange) - ) { - output.aggregates.set(precedingChange.id, change) - } else { - output.changes.push(change) - - const position = screenPosition(change) - if (position) { - output.positions.set(change.id, position) - } - } - } - - precedingChange = change - } - - if (threads) { - for (const comment of ranges.comments) { - if (isVisible(comment)) { - output.comments.push(comment) - if (!threads[comment.op.t]?.resolved) { - const position = screenPosition(comment) - if (position) { - output.positions.set(comment.id, position) - } - } - } + for (const change of aggregatedRanges.changes) { + const position = screenPosition(change) + if (position) { + positionsRef.current.set(change.id, position) } } - return output + for (const comment of aggregatedRanges.comments) { + const position = screenPosition(comment) + if (position) { + positionsRef.current.set(comment.id, position) + } + } }, - write(positionedRanges) { - setRangesWithPositions(positionedRanges) + write() { + setPositions(positionsRef.current) window.setTimeout(() => { containerRef.current?.dispatchEvent( new Event('review-panel:position') @@ -188,28 +188,22 @@ const ReviewPanelCurrentFile: FC = () => { }, }) } - }, [screenPosition, threads, view, ranges]) + }, [view, aggregatedRanges]) - useEffect(() => { - buildEntries() - }, [buildEntries]) + const showEmptyState = useMemo( + () => hasActiveRange(ranges, threads) === false, + [ranges, threads] + ) - useEventListener('editor:viewport-changed', buildEntries) - - if (!rangesWithPositions) { + if (!aggregatedRanges) { return null } - const showEmptyState = - threads && - rangesWithPositions.changes.length === 0 && - rangesWithPositions.comments.length === 0 - return (
{selectionCoords && (
{ {showEmptyState && } - {rangesWithPositions.changes.map(change => ( - - ))} + {aggregatedRanges.changes.map( + change => + positions.has(change.id) && ( + + ) + )} - {rangesWithPositions.comments.map(comment => ( - - ))} + {aggregatedRanges.comments.map( + comment => + positions.has(comment.id) && ( + + ) + )}
) } diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-header.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-header.tsx index b282e34d3c..172ea7483c 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-header.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-header.tsx @@ -6,16 +6,13 @@ import { Button } from 'react-bootstrap' import MaterialIcon from '@/shared/components/material-icon' import { useLayoutContext } from '@/shared/context/layout-context' -const ReviewPanelHeader: FC<{ - top: number - width: number -}> = ({ top, width }) => { +const ReviewPanelHeader: FC = () => { const [trackChangesMenuExpanded, setTrackChangesMenuExpanded] = useState(false) const { setReviewPanelOpen } = useLayoutContext() return ( -
+
Review