Merge pull request #16188 from overleaf/ii-rp-collapse-height

[web] Review panel comment container fix

GitOrigin-RevId: 0577949e711046303d25ba7e724564227d4a1bc7

services/web/frontend/js/features/source-editor/components/review-panel/entries/overview-file-entries.tsx

@@ -0,0 +1,96 @@
+import { useMemo } from 'react'
+import ChangeEntry from '@/features/source-editor/components/review-panel/entries/change-entry'
+import AggregateChangeEntry from '@/features/source-editor/components/review-panel/entries/aggregate-change-entry'
+import CommentEntry from '@/features/source-editor/components/review-panel/entries/comment-entry'
+import { useReviewPanelValueContext } from '@/features/source-editor/context/review-panel/review-panel-context'
+import {
+  ReviewPanelDocEntries,
+  ThreadId,
+} from '../../../../../../../types/review-panel/review-panel'
+import { ReviewPanelEntry } from '../../../../../../../types/review-panel/entry'
+import { DocId } from '../../../../../../../types/project-settings'
+
+type OverviewFileEntriesProps = {
+  docId: DocId
+  docEntries: ReviewPanelDocEntries
+}
+
+function OverviewFileEntries({ docId, docEntries }: OverviewFileEntriesProps) {
+  const { commentThreads, permissions, users } = useReviewPanelValueContext()
+
+  const objectEntries = useMemo(() => {
+    const entries = Object.entries(docEntries) as Array<
+      [ThreadId, ReviewPanelEntry]
+    >
+
+    const orderedEntries = entries.sort(([, entryA], [, entryB]) => {
+      return entryA.offset - entryB.offset
+    })
+
+    return orderedEntries
+  }, [docEntries])
+
+  return (
+    <div className="rp-overview-file-entries">
+      {objectEntries.map(([id, entry]) => {
+        if (entry.type === 'insert' || entry.type === 'delete') {
+          return (
+            <ChangeEntry
+              key={id}
+              docId={docId}
+              entryId={id}
+              permissions={permissions}
+              user={users[entry.metadata.user_id]}
+              content={entry.content}
+              offset={entry.offset}
+              type={entry.type}
+              focused={entry.focused}
+              entryIds={entry.entry_ids}
+              timestamp={entry.metadata.ts}
+            />
+          )
+        }
+
+        if (entry.type === 'aggregate-change') {
+          return (
+            <AggregateChangeEntry
+              key={id}
+              docId={docId}
+              entryId={id}
+              permissions={permissions}
+              user={users[entry.metadata.user_id]}
+              content={entry.content}
+              replacedContent={entry.metadata.replaced_content}
+              offset={entry.offset}
+              focused={entry.focused}
+              entryIds={entry.entry_ids}
+              timestamp={entry.metadata.ts}
+            />
+          )
+        }
+
+        if (entry.type === 'comment') {
+          const thread = commentThreads[entry.thread_id]
+          if (!thread?.resolved) {
+            return (
+              <CommentEntry
+                key={id}
+                docId={docId}
+                threadId={entry.thread_id}
+                thread={thread}
+                entryId={id}
+                offset={entry.offset}
+                focused={entry.focused}
+                permissions={permissions}
+              />
+            )
+          }
+        }
+
+        return null
+      })}
+    </div>
+  )
+}
+
+export default OverviewFileEntries

services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-collapse-height.ts

@@ -1,26 +0,0 @@
-import { useEffect } from 'react'
-
-function useCollapseHeight(
-  elRef: React.MutableRefObject<HTMLElement | null>,
-  shouldCollapse: boolean
-) {
-  useEffect(() => {
-    if (elRef.current) {
-      const neededHeight = elRef.current.scrollHeight
-
-      if (neededHeight > 0) {
-        const height = shouldCollapse ? 0 : neededHeight
-        // This might result in a too big height if the element has css prop of
-        // `box-sizing` set to `content-box`. To fix that, values of props such as
-        // box-sizing, padding and border could be extracted from `height` to compensate.
-        elRef.current.style.height = `${height}px`
-      } else {
-        if (shouldCollapse) {
-          elRef.current.style.height = '0'
-        }
-      }
-    }
-  }, [elRef, shouldCollapse])
-}
-
-export default useCollapseHeight

services/web/frontend/js/features/source-editor/components/review-panel/overview-file.tsx

@@ -1,17 +1,13 @@
-import { useMemo, useRef } from 'react'
+import { useMemo } from 'react'
 import Icon from '../../../../shared/components/icon'
-import ChangeEntry from './entries/change-entry'
-import AggregateChangeEntry from './entries/aggregate-change-entry'
-import CommentEntry from './entries/comment-entry'
 import {
   useReviewPanelUpdaterFnsContext,
   useReviewPanelValueContext,
 } from '../../context/review-panel/review-panel-context'
 import classnames from 'classnames'
-import { ThreadId } from '../../../../../../types/review-panel/review-panel'
+import { ReviewPanelDocEntries } from '../../../../../../types/review-panel/review-panel'
 import { MainDocument } from '../../../../../../types/project-settings'
-import { ReviewPanelEntry } from '../../../../../../types/review-panel/entry'
-import useCollapseHeight from './hooks/use-collapse-height'
+import OverviewFileEntries from '@/features/source-editor/components/review-panel/entries/overview-file-entries'
 
 type OverviewFileProps = {
   docId: MainDocument['doc']['id']
@@ -19,26 +15,13 @@ type OverviewFileProps = {
 }
 
 function OverviewFile({ docId, docPath }: OverviewFileProps) {
-  const { entries, collapsed, commentThreads, permissions, users } =
-    useReviewPanelValueContext()
+  const { entries, collapsed } = useReviewPanelValueContext()
   const { setCollapsed } = useReviewPanelUpdaterFnsContext()
 
   const docCollapsed = collapsed[docId]
-  const docEntries = useMemo(
-    () => (docId in entries ? entries[docId] : {}),
-    [docId, entries]
-  )
-  const objectEntries = useMemo(() => {
-    const entries = Object.entries(docEntries) as Array<
-      [ThreadId, ReviewPanelEntry]
-    >
-
-    const orderedEntries = entries.sort(([, entryA], [, entryB]) => {
-      return entryA.offset - entryB.offset
-    })
-
-    return orderedEntries
-  }, [docEntries])
+  const docEntries = useMemo(() => {
+    return docId in entries ? entries[docId] : ({} as ReviewPanelDocEntries)
+  }, [docId, entries])
   const entryCount = useMemo(() => {
     return Object.keys(docEntries).filter(
       key => key !== 'add-comment' && key !== 'bulk-actions'
@@ -49,9 +32,6 @@ function OverviewFile({ docId, docPath }: OverviewFileProps) {
     setCollapsed({ ...collapsed, [docId]: !docCollapsed })
   }
 
-  const entriesContainerRef = useRef<HTMLDivElement | null>(null)
-  useCollapseHeight(entriesContainerRef, docCollapsed)
-
   return (
     <div className="rp-overview-file">
       {entryCount > 0 && (
@@ -78,65 +58,9 @@ function OverviewFile({ docId, docPath }: OverviewFileProps) {
           )}
         </div>
       )}
-      <div className="rp-overview-file-entries" ref={entriesContainerRef}>
-        {objectEntries.map(([id, entry]) => {
-          if (entry.type === 'insert' || entry.type === 'delete') {
-            return (
-              <ChangeEntry
-                key={id}
-                docId={docId}
-                entryId={id}
-                permissions={permissions}
-                user={users[entry.metadata.user_id]}
-                content={entry.content}
-                offset={entry.offset}
-                type={entry.type}
-                focused={entry.focused}
-                entryIds={entry.entry_ids}
-                timestamp={entry.metadata.ts}
-              />
-            )
-          }
-
-          if (entry.type === 'aggregate-change') {
-            return (
-              <AggregateChangeEntry
-                key={id}
-                docId={docId}
-                entryId={id}
-                permissions={permissions}
-                user={users[entry.metadata.user_id]}
-                content={entry.content}
-                replacedContent={entry.metadata.replaced_content}
-                offset={entry.offset}
-                focused={entry.focused}
-                entryIds={entry.entry_ids}
-                timestamp={entry.metadata.ts}
-              />
-            )
-          }
-
-          if (entry.type === 'comment') {
-            const thread = commentThreads[entry.thread_id]
-            if (!thread?.resolved) {
-              return (
-                <CommentEntry
-                  key={id}
-                  docId={docId}
-                  threadId={entry.thread_id}
-                  thread={thread}
-                  entryId={id}
-                  offset={entry.offset}
-                  focused={entry.focused}
-                  permissions={permissions}
-                />
-              )
-            }
-          }
-
-          return null
-        })}
-      </div>
+      {!docCollapsed && (
+        <OverviewFileEntries docId={docId} docEntries={docEntries} />
+      )}
     </div>
   )
 }

services/web/frontend/js/features/source-editor/components/review-panel/toolbar/toggle-menu.tsx

@@ -1,15 +1,13 @@
-import { useState, useRef } from 'react'
-import { Trans, useTranslation } from 'react-i18next'
-import Tooltip from '../../../../../shared/components/tooltip'
+import { useState } from 'react'
+import { Trans } from 'react-i18next'
 import Icon from '../../../../../shared/components/icon'
-import TrackChangesToggle from './track-changes-toggle'
+import TrackChangesMenu from '@/features/source-editor/components/review-panel/toolbar/track-changes-menu'
 import UpgradeTrackChangesModal from '../upgrade-track-changes-modal'
 import { useProjectContext } from '../../../../../shared/context/project-context'
 import {
   useReviewPanelUpdaterFnsContext,
   useReviewPanelValueContext,
 } from '../../../context/review-panel/review-panel-context'
-import useCollapseHeight from '../hooks/use-collapse-height'
 import { send, sendMB } from '../../../../../infrastructure/event-tracking'
 import classnames from 'classnames'
 
@@ -21,30 +19,12 @@ const sendAnalytics = () => {
 }
 
 function ToggleMenu() {
-  const { t } = useTranslation()
   const project = useProjectContext()
-  const {
-    setShouldCollapse,
-    toggleTrackChangesForEveryone,
-    toggleTrackChangesForUser,
-    toggleTrackChangesForGuests,
-  } = useReviewPanelUpdaterFnsContext()
-  const {
-    permissions,
-    wantTrackChanges,
-    shouldCollapse,
-    trackChangesState,
-    trackChangesOnForEveryone,
-    trackChangesOnForGuests,
-    trackChangesForGuestsAvailable,
-    formattedProjectMembers,
-  } = useReviewPanelValueContext()
+  const { setShouldCollapse } = useReviewPanelUpdaterFnsContext()
+  const { wantTrackChanges, shouldCollapse } = useReviewPanelValueContext()
 
   const [showModal, setShowModal] = useState(false)
 
-  const containerRef = useRef<HTMLUListElement | null>(null)
-  useCollapseHeight(containerRef, shouldCollapse)
-
   const handleToggleFullTCStateCollapse = () => {
     if (project.features.trackChanges) {
       setShouldCollapse(value => !value)
@@ -88,108 +68,7 @@ function ToggleMenu() {
         </button>
       </span>
 
-      <ul
-        className="rp-tc-state"
-        ref={containerRef}
-        data-testid="review-panel-track-changes-menu"
-      >
-        <li className="rp-tc-state-item rp-tc-state-item-everyone">
-          <Tooltip
-            description={t('tc_switch_everyone_tip')}
-            id="track-changes-switch-everyone"
-            overlayProps={{
-              container: document.body,
-              placement: 'left',
-              delay: 1000,
-            }}
-          >
-            <span className="rp-tc-state-item-name">{t('tc_everyone')}</span>
-          </Tooltip>
-
-          <TrackChangesToggle
-            id="track-changes-everyone"
-            description={t('track_changes_for_everyone')}
-            handleToggle={() =>
-              toggleTrackChangesForEveryone(!trackChangesOnForEveryone)
-            }
-            value={trackChangesOnForEveryone}
-            disabled={!project.features.trackChanges || !permissions.write}
-          />
-        </li>
-        {Object.values(formattedProjectMembers).map(member => (
-          <li className="rp-tc-state-item" key={member.id}>
-            <Tooltip
-              description={t('tc_switch_user_tip')}
-              id="track-changes-switch-user"
-              overlayProps={{
-                container: document.body,
-                placement: 'left',
-                delay: 1000,
-              }}
-            >
-              <span
-                className={classnames('rp-tc-state-item-name', {
-                  'rp-tc-state-item-name-disabled': trackChangesOnForEveryone,
-                })}
-              >
-                {member.name}
-              </span>
-            </Tooltip>
-
-            <TrackChangesToggle
-              id={`track-changes-user-toggle-${member.id}`}
-              description={t('track_changes_for_x', { name: member.name })}
-              handleToggle={() =>
-                toggleTrackChangesForUser(
-                  !trackChangesState[member.id]?.value,
-                  member.id
-                )
-              }
-              value={Boolean(trackChangesState[member.id]?.value)}
-              disabled={
-                trackChangesOnForEveryone ||
-                !project.features.trackChanges ||
-                !permissions.write
-              }
-            />
-          </li>
-        ))}
-        <li className="rp-tc-state-separator" />
-        <li className="rp-tc-state-item">
-          <Tooltip
-            description={t('tc_switch_guests_tip')}
-            id="track-changes-switch-guests"
-            overlayProps={{
-              container: document.body,
-              placement: 'left',
-              delay: 1000,
-            }}
-          >
-            <span
-              className={classnames('rp-tc-state-item-name', {
-                'rp-tc-state-item-name-disabled': trackChangesOnForEveryone,
-              })}
-            >
-              {t('tc_guests')}
-            </span>
-          </Tooltip>
-
-          <TrackChangesToggle
-            id="track-changes-guests-toggle"
-            description="Track changes for guests"
-            handleToggle={() =>
-              toggleTrackChangesForGuests(!trackChangesOnForGuests)
-            }
-            value={trackChangesOnForGuests}
-            disabled={
-              trackChangesOnForEveryone ||
-              !project.features.trackChanges ||
-              !permissions.write ||
-              !trackChangesForGuestsAvailable
-            }
-          />
-        </li>
-      </ul>
+      {!shouldCollapse && <TrackChangesMenu />}
 
       <UpgradeTrackChangesModal show={showModal} setShow={setShowModal} />
     </>

services/web/frontend/js/features/source-editor/components/review-panel/toolbar/track-changes-menu.tsx

@@ -0,0 +1,130 @@
+import { useTranslation } from 'react-i18next'
+import Tooltip from '@/shared/components/tooltip'
+import TrackChangesToggle from '@/features/source-editor/components/review-panel/toolbar/track-changes-toggle'
+import {
+  useReviewPanelUpdaterFnsContext,
+  useReviewPanelValueContext,
+} from '@/features/source-editor/context/review-panel/review-panel-context'
+import { useProjectContext } from '@/shared/context/project-context'
+import classnames from 'classnames'
+
+function TrackChangesMenu() {
+  const { t } = useTranslation()
+  const project = useProjectContext()
+  const {
+    toggleTrackChangesForEveryone,
+    toggleTrackChangesForUser,
+    toggleTrackChangesForGuests,
+  } = useReviewPanelUpdaterFnsContext()
+  const {
+    permissions,
+    trackChangesState,
+    trackChangesOnForEveryone,
+    trackChangesOnForGuests,
+    trackChangesForGuestsAvailable,
+    formattedProjectMembers,
+  } = useReviewPanelValueContext()
+
+  return (
+    <ul className="rp-tc-state" data-testid="review-panel-track-changes-menu">
+      <li className="rp-tc-state-item rp-tc-state-item-everyone">
+        <Tooltip
+          description={t('tc_switch_everyone_tip')}
+          id="track-changes-switch-everyone"
+          overlayProps={{
+            container: document.body,
+            placement: 'left',
+            delay: 1000,
+          }}
+        >
+          <span className="rp-tc-state-item-name">{t('tc_everyone')}</span>
+        </Tooltip>
+
+        <TrackChangesToggle
+          id="track-changes-everyone"
+          description={t('track_changes_for_everyone')}
+          handleToggle={() =>
+            toggleTrackChangesForEveryone(!trackChangesOnForEveryone)
+          }
+          value={trackChangesOnForEveryone}
+          disabled={!project.features.trackChanges || !permissions.write}
+        />
+      </li>
+      {Object.values(formattedProjectMembers).map(member => (
+        <li className="rp-tc-state-item" key={member.id}>
+          <Tooltip
+            description={t('tc_switch_user_tip')}
+            id="track-changes-switch-user"
+            overlayProps={{
+              container: document.body,
+              placement: 'left',
+              delay: 1000,
+            }}
+          >
+            <span
+              className={classnames('rp-tc-state-item-name', {
+                'rp-tc-state-item-name-disabled': trackChangesOnForEveryone,
+              })}
+            >
+              {member.name}
+            </span>
+          </Tooltip>
+
+          <TrackChangesToggle
+            id={`track-changes-user-toggle-${member.id}`}
+            description={t('track_changes_for_x', { name: member.name })}
+            handleToggle={() =>
+              toggleTrackChangesForUser(
+                !trackChangesState[member.id]?.value,
+                member.id
+              )
+            }
+            value={Boolean(trackChangesState[member.id]?.value)}
+            disabled={
+              trackChangesOnForEveryone ||
+              !project.features.trackChanges ||
+              !permissions.write
+            }
+          />
+        </li>
+      ))}
+      <li className="rp-tc-state-separator" />
+      <li className="rp-tc-state-item">
+        <Tooltip
+          description={t('tc_switch_guests_tip')}
+          id="track-changes-switch-guests"
+          overlayProps={{
+            container: document.body,
+            placement: 'left',
+            delay: 1000,
+          }}
+        >
+          <span
+            className={classnames('rp-tc-state-item-name', {
+              'rp-tc-state-item-name-disabled': trackChangesOnForEveryone,
+            })}
+          >
+            {t('tc_guests')}
+          </span>
+        </Tooltip>
+
+        <TrackChangesToggle
+          id="track-changes-guests-toggle"
+          description="Track changes for guests"
+          handleToggle={() =>
+            toggleTrackChangesForGuests(!trackChangesOnForGuests)
+          }
+          value={trackChangesOnForGuests}
+          disabled={
+            trackChangesOnForEveryone ||
+            !project.features.trackChanges ||
+            !permissions.write ||
+            !trackChangesForGuestsAvailable
+          }
+        />
+      </li>
+    </ul>
+  )
+}
+
+export default TrackChangesMenu

services/web/frontend/stylesheets/app/editor/review-panel.less

@@ -1311,23 +1311,12 @@ button when (@is-overleaf-light = true) {
     height: 100%;
   }
 
-  .rp-overview-file {
-    .rp-overview-file-entries {
-      transition: height ease-in-out 0.15s;
-    }
-  }
-
   .rp-nav-item {
     border-right: 0;
     border-bottom: 0;
     border-left: 0;
     background: none;
   }
-
-  .rp-tc-state {
-    height: 0;
-    transition: height 150ms;
-  }
 }
 
 .rp-floating-entry {

services/web/test/frontend/features/review-panel/review-panel.spec.tsx

@@ -68,11 +68,11 @@ describe('<ReviewPanel />', function () {
 
       it('opens/closes toggle menu', function () {
         cy.get('@review-panel').within(() => {
-          cy.findByTestId('review-panel-track-changes-menu').as('menu')
-          cy.get('@menu').should('have.css', 'height', '1px')
+          cy.findByTestId('review-panel-track-changes-menu').should('not.exist')
           cy.findByRole('button', { name: /track changes is/i }).click()
           // verify the menu is expanded
-          cy.get('@menu')
+          cy.findByTestId('review-panel-track-changes-menu')
+            .as('menu')
             .then($el => {
               const height = window
                 .getComputedStyle($el[0])
@@ -81,7 +81,7 @@ describe('<ReviewPanel />', function () {
             })
             .should('be.gt', 1)
           cy.findByRole('button', { name: /track changes is/i }).click()
-          cy.get('@menu').should('have.css', 'height', '1px')
+          cy.get('@menu').should('not.exist')
         })
       })