ITEP-163854 - replace react virtuoso - p4 #230
Conversation
camiloHimura
requested review from
jpggvilaca,
MarkRedeman,
pplaskie,
ActiveChooN,
romanowska and
dwesolow
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR refactors scroll logic by introducing a new hook, useScrollToTargetItem, to centralize scrolling behavior in media item list components while also updating view mode settings for layout consistency.
- Replace react virtuoso scrolling with a custom scroll hook
- Update view mode settings (e.g., gap change for LARGE view mode)
- Refactor media and dataset list components (and their tests) to use the new hook
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web_ui/src/shared/components/media-view-modes/utils.ts | Updated view mode settings mapping and layout parameters |
| web_ui/src/shared/components/media-items-list/use-scroll-to-target-item.hook.tsx | Introduced a new hook for scrolling to a target item index |
| web_ui/src/shared/components/media-items-list/use-scroll-to-target-item.hook.test.tsx | Added tests for the new scrolling hook |
| web_ui/src/shared/components/media-items-list/media-items-list.component.tsx | Integrated the new hook into the media items list component |
| web_ui/src/pages/project-details/components/project-test/test-media-items-list.component.tsx | Updated test components to use the new scrollToIndex prop |
| web_ui/src/pages/project-details/components/project-media/media-content-bucket.component.tsx | Applied updated view mode settings for anomaly projects |
| web_ui/src/pages/annotator/components/sidebar/dataset/dataset-list.component.tsx | Refactored scrolling logic to leverage the new hook |
| web_ui/src/pages/annotator/components/sidebar/dataset/dataset-list-item-details.component.tsx | Enhanced media name display with truncation for better layout |
Comments suppressed due to low confidence (1)
web_ui/src/shared/components/media-view-modes/utils.ts:15
- Ensure that the changes in VIEW_MODE_SETTINGS—specifically swapping the positions of LARGE and SMALL and changing the gap for LARGE from 12 to 8—are intentional and aligned with the design requirements.
[ViewModes.LARGE]: { minItemSize: 300, gap: 8, maxColumns: 4 },
| const ref = useRef<HTMLDivElement | null>(null); | ||
| useLoadMore({ onLoadMore: endReached }, ref); | ||
|
|
||
| const container = ref?.current?.firstElementChild; |
There was a problem hiding this comment.
[nitpick] Consider moving the retrieval of the container element inside the useScrollToTargetItem hook or ensuring that it updates correctly, as accessing ref.current outside of the hook’s effect may lead to stale references.
Copilot uses AI. Check for mistakes.
| callback(scrollTo); | ||
| // we don't want to scroll immediately | ||
| // in case of changed view mode we have to scroll once view is rendered | ||
| }, 500); |
There was a problem hiding this comment.
a slow network might break this behavior. Could we not handle this on the parent? Maybe pass a "isReady" or "isEnabled" prop once the parent renders, perhaps inside a useEffect?
There was a problem hiding this comment.
The hook re-executes when scrollToIndex changes. Once the items are loaded and the index is set, it responds as expected. I tested it on a 3G connection, and it worked fine
|
|
||
| import { useScrollToTargetItem } from './use-scroll-to-target-item.hook'; | ||
|
|
||
| jest.useFakeTimers(); |
There was a problem hiding this comment.
Could we do it in the beforeEach? I think we also miss useRealTimers, we might add it to afterEach.
| @@ -12,8 +12,8 @@ export const INITIAL_VIEW_MODE = ViewModes.MEDIUM; | |||
| export const VIEW_MODE_LABEL = 'View mode'; | |||
|
|
|||
| export const VIEW_MODE_SETTINGS = { | |||
| [ViewModes.SMALL]: { minItemSize: 112, gap: 4, maxColumns: 11 }, | |||
| [ViewModes.LARGE]: { minItemSize: 300, gap: 8, maxColumns: 4 }, | |||
There was a problem hiding this comment.
gap for the LARGE mode was 12 before, is it intentional change?
📝 Description
Add a new hook to scroll a dataset to a specific item index. This will centralize the functionality within MediaItemsList, reducing duplicated code
Dataset List/annotator:
Screen.Recording.2025-05-16.at.10.38.43.mov
Test Dataset List/preview:
Screen.Recording.2025-05-16.at.10.42.56.mov
If referencing an internal ticket (e.g. JIRA), include the ticket number instead:
https://jira.devtools.intel.com/browse/ITEP-32688
-->
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: