Skip to content

Conversation

@severo
Copy link
Contributor

@severo severo commented Jan 15, 2026

We already scroll to the current cell using scrollIntoView() when focusing it. No need to do the job twice.

On the contrary, when using the virtual scroll, we need to operate in two steps:

  1. scroll vertically to the low-precision scrollTop, so that the current row is in the viewport
  2. once done (isScrolling === false), we use scrollIntoView to scroll horizontally

We cannot use scrollIntoView for point 1. because the new scrollTop would not be aligned with the computed state.

We already scroll using scrollintoView() when focusing the current cell.
No need to do the job twice.
@severo severo merged commit 143e6a0 into master Jan 15, 2026
5 checks passed
@severo severo requested a review from Copilot January 15, 2026 09:24
@severo severo deleted the command-scrollTop branch January 15, 2026 09:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the native scroll mode to eliminate redundant scrolling operations. Instead of using both scrollTo() and scrollIntoView(), the native mode now relies solely on scrollIntoView() for scrolling to cells, while the virtual scroll mode continues to use scrollTo() for low-precision vertical scrolling followed by scrollIntoView() for horizontal scrolling.

Changes:

  • Removed scrollTo state and setScrollTo from ScrollModeNativeProvider
  • Modified scrollRowIntoView in native mode to update scrollTop state instead of calling scrollTo()
  • Added headerHeight parameter to ScrollModeNativeProvider to correctly calculate scroll positions
  • Enhanced documentation in useCellFocus explaining the preventScroll usage

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/providers/ScrollModeProvider.tsx Passes headerHeight prop to ScrollModeNativeProvider
src/providers/ScrollModeNativeProvider.tsx Removes scrollTo mechanism and uses state-based scrolling instead
src/hooks/useCellFocus.ts Adds preventScroll option and improves documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant