Merge virtual and native scroll modes into one #392
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR merges the separate native and virtual scroll modes into a unified scrolling implementation that handles both cases automatically based on the number of rows. The key changes consolidate three provider files and two test files into single unified versions, and add CSS overflow clipping.
Changes:
- Consolidated
ScrollModeVirtualProvider,ScrollModeNativeProvider, andScrollModeProviderinto a singleScrollProvider - Merged
virtualScroll.tshelper intoscroll.tswith unified logic that uses a scale factor (1 for normal mode, >1 for virtual mode) - Updated all context references from
ScrollModeContexttoScrollContextand removed thescrollModefield from the context type
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/helpers/virtualScroll.test.ts | Removed old virtual scroll tests (file deleted) |
| test/helpers/scroll.test.ts | Added comprehensive unified tests covering both normal and virtual scroll modes |
| src/providers/ScrollProvider.tsx | Consolidated provider that auto-selects mode based on maxElementHeight |
| src/providers/ScrollModeProvider.tsx | Removed old mode-switching provider (file deleted) |
| src/providers/ScrollModeNativeProvider.tsx | Removed old native-only provider (file deleted) |
| src/providers/RowsAndColumnsProvider.tsx | Updated import from ScrollModeContext to ScrollContext |
| src/hooks/useCellFocus.ts | Updated import and improved comments about scroll behavior |
| src/helpers/scroll.ts | Unified scroll logic with scale factor approach (factor=1 for normal, factor>1 for virtual) |
| src/contexts/ScrollContext.ts | Renamed from ScrollModeContext, removed scrollMode field from type |
| src/components/HighTable/Wrapper.tsx | Updated to use ScrollProvider instead of ScrollModeProvider |
| src/components/HighTable/Slice.tsx | Updated import from ScrollModeContext to ScrollContext |
| src/components/HighTable/Scroller.tsx | Updated import from ScrollModeContext to ScrollContext |
| src/components/HighTable/HighTable.test.tsx | Updated test expectations for row indices (off-by-one fix) |
| src/HighTable.module.css | Added overflow-x: visible and overflow-y: clip to handle vertical overflow |
Comments suppressed due to low confidence (2)
src/providers/ScrollProvider.tsx:72
- The
scrollModefield is being returned in the context value but was removed from theScrollContextTypeinterface definition inScrollContext.ts. Either add this field back to the interface or remove it from the returned value. Since the PR aims to merge modes into one, removing this field seems more appropriate.
src/helpers/scroll.ts:124 - Commented-out code should be removed. If this alternative implementation is needed for reference or future consideration, it should be documented in a TODO comment or removed entirely.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
severo
force-pushed
the
one-scroll-mode
branch
from
0b45bce to
ccdc316
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR merges the native and virtual modes into a single mode that handles both cases.
I also fixed the CSS to clip the Y overflow: it might fix #389. Can you re-test, @platypii?