From 29c5bc8206ae130e01bdb54ed131eaf8eecdb23f Mon Sep 17 00:00:00 2001
From: Domagoj Kriskovic <dom.kriskovic@overleaf.com>
Date: Fri, 25 Oct 2024 15:05:44 +0200
Subject: [PATCH] Focused and selected states for ReviewPanelEntry (#21354)

GitOrigin-RevId: 41e5ed1110d71eae51cbbde1e874dda133d666c8
---
 .../components/review-panel-entry.tsx          | 18 ++++++++++++++++--
 .../review-panel-new/utils/position-items.ts   |  6 +++---
 .../app/editor/review-panel-new.less           |  6 +++++-
 .../pages/editor/review-panel-new.scss         |  6 +++++-
 4 files changed, 29 insertions(+), 7 deletions(-)

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
index 69f485380e..203e91194a 100644
--- 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
@@ -38,6 +38,7 @@ export const ReviewPanelEntry: FC<{
   const state = useCodeMirrorStateContext()
   const view = useCodeMirrorViewContext()
   const { openDocId, getCurrentDocId } = useEditorManagerContext()
+  const [selected, setSelected] = useState(false)
   const [focused, setFocused] = useState(false)
   const { setReviewPanelOpen } = useLayoutContext()
 
@@ -49,6 +50,8 @@ export const ReviewPanelEntry: FC<{
 
   const focusHandler = useCallback(
     event => {
+      setFocused(true)
+
       if (
         event.target instanceof HTMLButtonElement ||
         event.target instanceof HTMLLinkElement ||
@@ -59,7 +62,7 @@ export const ReviewPanelEntry: FC<{
         return
       }
 
-      setFocused(true)
+      setSelected(true)
 
       if (!selectLineOnFocus) {
         return
@@ -97,7 +100,10 @@ export const ReviewPanelEntry: FC<{
     <div
       onMouseDown={openReviewPanel} // Using onMouseDown rather than onClick to guarantee that it fires before onFocus
       onFocus={focusHandler}
-      onBlur={() => setFocused(false)}
+      onBlur={() => {
+        setSelected(false)
+        setFocused(false)
+      }}
       onMouseEnter={() => {
         if (hoverRanges) {
           view.dispatch(highlightRanges(op))
@@ -113,7 +119,15 @@ export const ReviewPanelEntry: FC<{
       className={classNames(
         'review-panel-entry',
         {
+          // 'selected' is used to manually select an entry
+          // useful if the range is within range and you want to show the one outside the viewport
+          // it is not enough to just check isSelectionWithinOp for that
+          'review-panel-entry-selected': selected,
+          // 'focused' is set even when an entry was clicked but not selected (like clicking on a menu option)
+          // used to set z-index above other entries (since entries are not ordered the same way visually and in the DOM)
           'review-panel-entry-focused': focused,
+          // 'highlighted' is set if the selection is within op but that doesn't necessarily mean it should be selected
+          // multiple entries can be highlighted at the same time
           'review-panel-entry-highlighted': highlighted,
           'review-panel-entry-disabled': disabled,
         },
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 7982312f04..10f2d86ca4 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
@@ -26,14 +26,14 @@ export const positionItems = debounce(
 
     if (activeItemIndex === -1) {
       // if there is no action available
-      // check if there is a focused entry
+      // check if there is manually selected entry
       activeItemIndex = items.findIndex(item =>
-        item.classList.contains('review-panel-entry-focused')
+        item.classList.contains('review-panel-entry-selected')
       )
     }
 
     if (activeItemIndex === -1) {
-      // if entry was not focused manually
+      // if entry was not selected manually
       // check if there is an entry in selection and use that as the focused item
       activeItemIndex = items.findIndex(item =>
         item.classList.contains('review-panel-entry-highlighted')
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 c74d0ed230..6387c6a327 100644
--- a/services/web/frontend/stylesheets/app/editor/review-panel-new.less
+++ b/services/web/frontend/stylesheets/app/editor/review-panel-new.less
@@ -51,7 +51,7 @@
     gap: @spacing-04;
   }
 
-  .review-panel-entry.review-panel-entry-focused,
+  .review-panel-entry.review-panel-entry-selected,
   .review-panel-entry.review-panel-entry-highlighted {
     margin-left: @spacing-01;
     border: 1px solid @blue-50;
@@ -61,6 +61,10 @@
       0px 2px 4px rgba(30, 37, 48, 0.08);
   }
 
+  .review-panel-entry.review-panel-entry-focused {
+    z-index: 2;
+  }
+
   .review-panel-entry-header {
     display: flex;
     justify-content: space-between;
diff --git a/services/web/frontend/stylesheets/bootstrap-5/pages/editor/review-panel-new.scss b/services/web/frontend/stylesheets/bootstrap-5/pages/editor/review-panel-new.scss
index e09bbdab5b..6d1fb6b4ad 100644
--- a/services/web/frontend/stylesheets/bootstrap-5/pages/editor/review-panel-new.scss
+++ b/services/web/frontend/stylesheets/bootstrap-5/pages/editor/review-panel-new.scss
@@ -56,7 +56,7 @@
     gap: var(--spacing-04);
   }
 
-  .review-panel-entry.review-panel-entry-focused,
+  .review-panel-entry.review-panel-entry-selected,
   .review-panel-entry.review-panel-entry-highlighted {
     margin-left: var(--spacing-01);
     border: 1px solid var(--border-active);
@@ -64,6 +64,10 @@
     @include shadow-md;
   }
 
+  .review-panel-entry.review-panel-entry-focused {
+    z-index: 2;
+  }
+
   .review-panel-entry-header {
     display: flex;
     justify-content: space-between;