-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: offset calculation to handle transformed elements with translateY or translateX in scrollIntoView #7717
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
fix: offset calculation to handle transformed elements with translateY or translateX in scrollIntoView #7717
Conversation
hshoja
left a comment
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.
Looks great, tested with tanstack virtuliazer and gridlist from react aria 👍🏼
|
Thanks for the PR! Is there an existing story we can test this with? If not, could you create a new one? |
@reidbarber I couldn't find an existing story that demonstrates this issue, so I created a simple example with reproducible steps. Please check it out https://stackblitz.com/edit/vitejs-vite-zmdnq3jq. |
| child = child.offsetParent as HTMLElement; | ||
| } | ||
| return sum; | ||
| let childRect = child.getBoundingClientRect(); |
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.
We're a little nervous about this change. We think it's missing some cases. Looking at some other libraries, there is a fair amount of complexity in determining this. For example https://www.npmjs.com/package/compute-scroll-into-view
We might be able to use this library, though are reluctant to bring in the size increase. So the thing we need to prove is that this change is "enough" for our use cases or decide we're ok with the size increase.
|
@snowystinger Hello! Can we get a chance to get this merged? I just hit the issue when I tried to use DatePicker inside a virtually scrolled container. I really need this fix.
I understand this concern, but I would like to emphasize that the current implementation is actually broken in some sense. The new I'd appreciate if you could revisit this change. @piecyk Thank you for the fix! |
|
Thanks, I'll bump with the team to see if we can more concretely define what "enough" means. If anyone has any ideas on how to prove this, we'd welcome the ideas |
devongovett
left a comment
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.
Approving for testing. We'll see if we find any regressions.
yihuiliao
left a comment
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.
approving for testing
Closes #7716
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: