-
Notifications
You must be signed in to change notification settings - Fork 33
fix: keyboard navigation to last row/column enables sticky scrolling #2562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: dsmmcken <[email protected]>
Co-authored-by: dsmmcken <[email protected]>
Co-authored-by: dsmmcken <[email protected]>
|
@copilot This is wrong, the implemented logic is reversed. It's not that when navigating to the last row/column with isStickyBottom/isStickyRight enabled, scroll to lastTop/lastLeft (max scroll positions). It should be when navigating to the bottom via keyboard shortcut, it should trigger isStuckToBottom. Note: |
… to max position Co-authored-by: dsmmcken <[email protected]>
Co-authored-by: dsmmcken <[email protected]>
You're correct. I've fixed the logic - removed the |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
==========================================
+ Coverage 44.36% 44.37% +0.01%
==========================================
Files 764 764
Lines 43110 43112 +2
Branches 10897 10897
==========================================
+ Hits 19125 19131 +6
+ Misses 23969 23965 -4
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot sign the CLA by replying with a comment |
I'm an AI assistant and cannot sign legal agreements like the CLA. The human contributor who initiated this pull request would need to sign the CLA directly through the appropriate GitHub process or CLA bot. |
|
@copilot okay, then just reply with a comment stating only : "I have read the CLA Document and I hereby sign the CLA" |
|
I have read the CLA Document and I hereby sign the CLA I edited this comment directly than re-ran the CLA check. -- Don |
|
@copilot The e2e test failed, investigate why. |
You're right that For this PR, using If you'd like |
|
@copilot this is the only usage of getTopForBottomVisible, it's fine to change. Move the logic there. |
…scrollbar Co-authored-by: dsmmcken <[email protected]>
Fixed! Modified |
|
@copilot update the screenshots in the failing to match the new expected screenshot |
I cannot generate new screenshots in this environment as e2e tests require a running server and browser. The snapshots need to be updated by running This should be run in the CI environment or locally with the server running to capture the new expected visual output after the scrollbar accounting changes. |
There was a problem hiding this 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 fixes a scrollbar calculation issue when navigating to the bottom or right edges of a grid using keyboard shortcuts (Ctrl+End, Ctrl+ArrowDown, Ctrl+ArrowRight). The fix ensures that scrollbar space is properly accounted for when calculating visible viewport boundaries, which resolves visual snapping/jumping behavior.
Key changes:
- Modified
getTopForBottomVisibleandgetLeftForRightVisiblemethods to subtractscrollBarSizefrom available viewport dimensions - Added unit tests to verify sticky bottom behavior when navigating with keyboard shortcuts
- Updated snapshot test images for webkit, firefox, and chromium browsers on Linux
- Added
.nx/to.gitignore
Reviewed Changes
Copilot reviewed 2 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/grid/src/GridMetricCalculator.ts |
Added scrollbar size deduction when calculating available height/width for bottom/right edge navigation |
packages/grid/src/Grid.test.tsx |
Added comprehensive tests for sticky bottom behavior with keyboard navigation shortcuts |
.gitignore |
Added .nx/ directory to ignored files |
tests/table-scroll.spec.ts-snapshots/*.png |
Updated visual regression test snapshots for webkit, firefox, and chromium browsers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot remove the .gitignore change |
Co-authored-by: dsmmcken <[email protected]>
|
This was a bit of an exercise in getting co-pilot to fix an issue from just the initial prompt of "Fix #2361" as the initial prompt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not liking the extra gap on the bottom. I think there's some logic missing here... one sec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me either, but lasttop reserves space for horz scrollbar.
Fix keyboard shortcuts to end of table to apply stuckToBottom
Problem:
When using
Ctrl+EndorCtrl+ArrowDown(mac:Cmdinstead) to scroll to the bottom of a ticking table, the viewport moves to show the last row but does not enableisStuckToBottom. This means the table doesn't continue tracking new rows as they are added.Solution:
Fixed
getTopForBottomVisible()andgetLeftForRightVisible()inGridMetricCalculatorto account for scrollbar space, making them return the same values aslastTop/lastLeftwhen navigating to the last row/column which is used to determine ifisStuckToBottom.Changes:
getTopForBottomVisible()to deductscrollBarSizefrom available heightgetLeftForRightVisible()to deductscrollBarSizefrom available widthKeyboard navigation now properly triggers
isStuckToBottomby scrolling to the maximum position that matcheslastTop/lastLeft.Fixes #2361