-
Notifications
You must be signed in to change notification settings - Fork 345
Fix scrolling by pixels #951
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
base: main
Are you sure you want to change the base?
Conversation
@jassmith for merge? |
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 addresses an issue with scrolling by pixels by modifying both the example story and the core scrolling behavior.
- In the story file, it adds a button to scroll horizontally by a fixed pixel amount.
- In the data editor file, it adjusts the scroll initialization to use provided desired scroll positions.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/core/src/docs/examples/imperative-scroll.stories.tsx | Adds a button and handler to scroll horizontally by a fixed pixel value, demonstrating pixel-based scrolling. |
packages/core/src/data-editor/data-editor.tsx | Updates the default scroll positions to use external desired values for improved scrolling accuracy. |
Comments suppressed due to low confidence (1)
packages/core/src/data-editor/data-editor.tsx:1499
- [nitpick] The change to use desiredX and desiredY improves flexibility; consider adding a brief comment to clarify the intended behavior and usage of these external scroll position values.
let scrollX = desiredX ?? 0;
const pixels = 1_000_000_000; | ||
const onClickEnd = () => { | ||
ref.current?.scrollTo({ | ||
amount: pixels, |
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.
[nitpick] The fixed pixel value of 1,000,000,000 is extreme and may produce inconsistent behavior on different devices. Consider calculating the scroll offset dynamically based on the scroll container’s dimensions to ensure reliable behavior.
const pixels = 1_000_000_000; | |
const onClickEnd = () => { | |
ref.current?.scrollTo({ | |
amount: pixels, | |
const onClickEnd = () => { | |
const containerWidth = ref.current?.getBoundingClientRect()?.width || 0; | |
const maxScrollOffset = containerWidth * 2; // Example: Scroll twice the container's width | |
ref.current?.scrollTo({ | |
amount: maxScrollOffset, |
Copilot uses AI. Check for mistakes.
Peek.2024-05-09.10-48.mp4