From ad7abee39a8bb6020cd8da58c3e44f7fa648e96c Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Thu, 15 Aug 2024 12:07:40 +0200 Subject: [PATCH] Remove extra padding for off-screen review panel entries (#19841) * Remove extra padding for off-screen review panel entries * use document.activeElement * cleanup editor padding cases * using isSelectionWithinOp * fix formatting * focusHandler function * 1px border for focused entry * use constants * using isFirstEntry GitOrigin-RevId: 4509f803b6cb907b40f1745a6fc7f3b1edfe145c --- .../components/review-panel-change.tsx | 23 +++---- .../components/review-panel-comment.tsx | 31 +++------- .../components/review-panel-current-file.tsx | 34 +---------- .../components/review-panel-entry.tsx | 61 +++++++++++++++++++ ...s-focused.ts => is-selection-within-op.ts} | 5 +- .../review-panel-new/utils/position-items.ts | 40 +++++++----- .../app/editor/review-panel-new.less | 5 +- 7 files changed, 111 insertions(+), 88 deletions(-) create mode 100644 services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx rename services/web/frontend/js/features/review-panel-new/utils/{is-focused.ts => is-selection-within-op.ts} (71%) diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-change.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-change.tsx index eecb363a5c..3e64c1c75c 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-change.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-change.tsx @@ -7,22 +7,20 @@ import { } from '../../../../../types/change' import { useTranslation } from 'react-i18next' import classnames from 'classnames' -import { useCodeMirrorStateContext } from '@/features/source-editor/components/codemirror-editor' import { usePermissionsContext } from '@/features/ide-react/context/permissions-context' -import { isFocused } from '../utils/is-focused' import { Button } from 'react-bootstrap' import Tooltip from '@/shared/components/tooltip' import MaterialIcon from '@/shared/components/material-icon' import { formatTimeBasedOnYear } from '@/features/utils/format-date' import { useChangesUsersContext } from '../context/changes-users-context' import { ReviewPanelChangeUser } from './review-panel-change-user' +import { ReviewPanelEntry } from './review-panel-entry' export const ReviewPanelChange = memo<{ change: Change aggregate?: Change top?: number }>(({ change, aggregate, top }) => { - const state = useCodeMirrorStateContext() const { t } = useTranslation() const { acceptChanges, rejectChanges } = useRangesActionsContext() const permissions = usePermissionsContext() @@ -33,23 +31,16 @@ export const ReviewPanelChange = memo<{ return null } - const focused = isFocused(change.op, state.selection.main) - return ( -
@@ -166,7 +157,7 @@ export const ReviewPanelChange = memo<{ )}
- + ) }) ReviewPanelChange.displayName = 'ReviewPanelChange' diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-comment.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-comment.tsx index c136d1dc6b..7d8b04da52 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-comment.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-comment.tsx @@ -6,11 +6,10 @@ import { useThreadsActionsContext, useThreadsContext, } from '../context/threads-context' -import { useCodeMirrorStateContext } from '@/features/source-editor/components/codemirror-editor' import classnames from 'classnames' -import { isFocused } from '../utils/is-focused' import AutoExpandingTextArea from '@/shared/components/auto-expanding-text-area' import MaterialIcon from '@/shared/components/material-icon' +import { ReviewPanelEntry } from './review-panel-entry' export const ReviewPanelComment = memo<{ comment: Change @@ -22,7 +21,6 @@ export const ReviewPanelComment = memo<{ const [content, setContent] = useState('') const threads = useThreadsContext() const { resolveThread, addMessage } = useThreadsActionsContext() - const state = useCodeMirrorStateContext() const handleSubmitReply = useCallback(() => { setSubmitting(true) @@ -52,25 +50,14 @@ export const ReviewPanelComment = memo<{ return null } - const focused = isFocused(comment.op, state.selection.main) - return ( -
@@ -109,7 +96,7 @@ export const ReviewPanelComment = memo<{ {error &&
{error.message}
}
-
+ ) }) ReviewPanelComment.displayName = 'ReviewPanelComment' 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 c02b4ef0b8..0ecdb9cb34 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,10 +16,7 @@ import { DeleteOperation, EditOperation, } from '../../../../../types/change' -import { - editorOverflowPadding, - editorVerticalTopPadding, -} from '@/features/source-editor/extensions/vertical-overflow' +import { editorVerticalTopPadding } from '@/features/source-editor/extensions/vertical-overflow' import { useCodeMirrorStateContext, useCodeMirrorViewContext, @@ -77,50 +74,25 @@ const ReviewPanelCurrentFile: FC = () => { ) const containerRef = useRef(null) - const ignoreNextUpdateRef = useRef(false) const previousFocusedItem = useRef(0) const updatePositions = useCallback(() => { - if (ignoreNextUpdateRef.current) { - ignoreNextUpdateRef.current = false - return - } - if (containerRef.current) { const extents = positionItems( containerRef.current, - view.scrollDOM as HTMLDivElement, previousFocusedItem.current ) if (extents) { previousFocusedItem.current = extents.focusedItemIndex - - window.setTimeout(() => { - const top = extents.min < 0 ? -extents.min : 0 - const bottom = - extents.max > contentRect.bottom - ? extents.max - contentRect.bottom - : 0 - - const currentPadding = editorOverflowPadding(view) - - if ( - currentPadding?.top !== top || - currentPadding?.bottom !== bottom - ) { - // ignoreNextUpdateRef.current = true - // view.dispatch(setVerticalOverflow({ top, bottom })) - } - }) } } - }, [contentRect.bottom, view]) + }, []) useEffect(() => { const timer = window.setTimeout(() => { updatePositions() - }, 100) + }, 50) return () => { window.clearTimeout(timer) diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx new file mode 100644 index 0000000000..527912c637 --- /dev/null +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx @@ -0,0 +1,61 @@ +import { FC, useCallback, useState } from 'react' +import { AnyOperation } from '../../../../../types/change' +import { + useCodeMirrorStateContext, + useCodeMirrorViewContext, +} from '@/features/source-editor/components/codemirror-editor' +import { isSelectionWithinOp } from '../utils/is-selection-within-op' +import { EditorSelection } from '@codemirror/state' +import { EditorView } from '@codemirror/view' +import classNames from 'classnames' + +export const ReviewPanelEntry: FC<{ + position: number + op: AnyOperation + top?: number + className?: string +}> = ({ children, position, top, op, className }) => { + const state = useCodeMirrorStateContext() + const view = useCodeMirrorViewContext() + const [focused, setFocused] = useState(false) + + const highlighted = isSelectionWithinOp(op, state.selection.main) + + const focusHandler = useCallback(() => { + setTimeout(() => { + // without setTimeout, error "EditorView.update are not allowed while an update is in progress" can occur + // this can be avoided by using onClick rather than onFocus but it will then not pick up or events for focusing entries + view.dispatch({ + selection: EditorSelection.cursor(position), + effects: EditorView.scrollIntoView(position, { y: 'center' }), + }) + }, 0) + setFocused(true) + }, [view, position]) + + return ( +
setFocused(false)} + role="button" + tabIndex={position + 1} + className={classNames( + 'review-panel-entry', + { + 'review-panel-entry-focused': focused, + 'review-panel-entry-highlighted': highlighted, + }, + className + )} + data-top={top} + data-pos={position} + style={{ + position: top === undefined ? 'relative' : 'absolute', + visibility: top === undefined ? 'visible' : 'hidden', + transition: 'top .3s, left .1s, right .1s', + }} + > + {children} +
+ ) +} diff --git a/services/web/frontend/js/features/review-panel-new/utils/is-focused.ts b/services/web/frontend/js/features/review-panel-new/utils/is-selection-within-op.ts similarity index 71% rename from services/web/frontend/js/features/review-panel-new/utils/is-focused.ts rename to services/web/frontend/js/features/review-panel-new/utils/is-selection-within-op.ts index 13263322d5..6d91d9aece 100644 --- a/services/web/frontend/js/features/review-panel-new/utils/is-focused.ts +++ b/services/web/frontend/js/features/review-panel-new/utils/is-selection-within-op.ts @@ -2,6 +2,9 @@ import { AnyOperation } from '../../../../../types/change' import { SelectionRange } from '@codemirror/state' import { visibleTextLength } from '@/utils/operations' -export const isFocused = (op: AnyOperation, range: SelectionRange): boolean => { +export const isSelectionWithinOp = ( + op: AnyOperation, + range: SelectionRange +): boolean => { return range.to >= op.p && range.from <= op.p + visibleTextLength(op) } diff --git a/services/web/frontend/js/features/review-panel-new/utils/position-items.ts b/services/web/frontend/js/features/review-panel-new/utils/position-items.ts index f22017aab6..f0e3a5cc6a 100644 --- a/services/web/frontend/js/features/review-panel-new/utils/position-items.ts +++ b/services/web/frontend/js/features/review-panel-new/utils/position-items.ts @@ -1,13 +1,10 @@ import { debounce } from 'lodash' -export const positionItems = debounce( - ( - element: HTMLDivElement, - containerElement: HTMLDivElement, - previousFocusedItemIndex: number - ) => { - const scrollRect = containerElement.getBoundingClientRect() +const COLLAPSED_HEADER_HEIGHT = 75 +const OFFSET_FOR_ENTRIES_ABOVE = 70 +export const positionItems = debounce( + (element: HTMLDivElement, previousFocusedItemIndex: number) => { const items = Array.from( element.querySelectorAll('.review-panel-entry') ) @@ -21,19 +18,25 @@ export const positionItems = debounce( let focusedItemIndex = items.findIndex(item => item.classList.contains('review-panel-entry-focused') ) + if (focusedItemIndex === -1) { + // if entry was not focused manually + // check if there is an entry in selection and use that as the focused item + focusedItemIndex = items.findIndex(item => + item.classList.contains('review-panel-entry-highlighted') + ) + } if (focusedItemIndex === -1) { focusedItemIndex = previousFocusedItemIndex } - // TODO: editorPadding? - const topDiff = scrollRect.top - 80 - const focusedItem = items[focusedItemIndex] if (!focusedItem) { return } - const focusedItemTop = Number(focusedItem.dataset.top) - focusedItem.style.top = `${focusedItemTop + topDiff}px` + + const focusedItemTop = getTopPosition(focusedItem, focusedItemIndex === 0) + + focusedItem.style.top = `${focusedItemTop}px` focusedItem.style.visibility = 'visible' const focusedItemRect = focusedItem.getBoundingClientRect() @@ -42,12 +45,12 @@ export const positionItems = debounce( for (let i = focusedItemIndex - 1; i >= 0; i--) { const item = items[i] const rect = item.getBoundingClientRect() - let top = Number(item.dataset.top) + let top = getTopPosition(item, i === 0) const bottom = top + rect.height if (bottom > topLimit) { top = topLimit - rect.height - 10 } - item.style.top = `${top + topDiff}px` + item.style.top = `${top}px` item.style.visibility = 'visible' topLimit = top } @@ -57,11 +60,11 @@ export const positionItems = debounce( for (let i = focusedItemIndex + 1; i < items.length; i++) { const item = items[i] const rect = item.getBoundingClientRect() - let top = Number(item.dataset.top) + let top = getTopPosition(item, false) if (top < bottomLimit) { top = bottomLimit + 10 } - item.style.top = `${top + topDiff}px` + item.style.top = `${top}px` item.style.visibility = 'visible' bottomLimit = top + rect.height } @@ -75,3 +78,8 @@ export const positionItems = debounce( 100, { leading: false, trailing: true, maxWait: 1000 } ) + +function getTopPosition(item: HTMLDivElement, isFirstEntry: boolean) { + const offset = isFirstEntry ? 0 : OFFSET_FOR_ENTRIES_ABOVE + return Math.max(COLLAPSED_HEADER_HEIGHT + offset, Number(item.dataset.top)) +} diff --git a/services/web/frontend/stylesheets/app/editor/review-panel-new.less b/services/web/frontend/stylesheets/app/editor/review-panel-new.less index 9a254e7da1..14a19d8e6a 100644 --- a/services/web/frontend/stylesheets/app/editor/review-panel-new.less +++ b/services/web/frontend/stylesheets/app/editor/review-panel-new.less @@ -33,9 +33,10 @@ gap: @spacing-04; } - .review-panel-entry-focused { + .review-panel-entry-focused, + .review-panel-entry-highlighted { margin-left: @spacing-01; - border: 2px solid @blue-50; + border: 1px solid @blue-50; } .review-panel-entry-header { display: flex;