Fix #4290 and #4289: Time component scroll issue #4296
Conversation
This commit addresses an issue where the scroll position was not accurately set when the component mounted. To resolve this, we have incorporated the use of requestAnimationFrame to delay the scroll update until after the component has fully rendered and layout calculations are complete. This ensures consistent and precise scroll behavior. Closes Hacker0x01#4290
In the time component, the scroll position is set using requestAnimationFrame. To ensure the accurate testing, this commit updates the test case to use waitFor from React Testing Library, allowing the test to wait for the scroll update to complete before making assertions about the scroll position. The test now provides more reliable results and ensures that the scroll behavior is properly tested. Closes Hacker0x01#4290
…Frame In the Time component, this commit refactors the code to ensure that the component ref is available when used inside a requestAnimationFrame callback. By checking the ref availability, the issue of rendering failures is resolved, particularly during testing with asynchronous behavior. Closes Hacker0x01#4290
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@balajis-qb you can click here to see the review status or cancel the code review job.
Codecov Report
@@ Coverage Diff @@
## main #4296 +/- ##
=======================================
Coverage 96.52% 96.52%
=======================================
Files 25 25
Lines 2358 2362 +4
Branches 962 963 +1
=======================================
+ Hits 2276 2280 +4
Misses 82 82
|
balajis-qb
changed the title
balajis-qb
changed the title
There was a problem hiding this comment.
Thanks for writing such a great PR description. It looks like the scroll logic changes and async tests should do what's expected.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
overall changes look good and consistent with the described changes. Testing has been updated as appropriate, no issues are noted.
Reviewed with ❤️ by PullRequest
| : this.list.clientHeight, | ||
| this.centerLi, | ||
| ); | ||
| this.scrollToTheSelectedTime(); |
There was a problem hiding this comment.
Thanks for extracting this logic
|
Thank you for the review and approving my PR |
Closes #4290 and #4289
Summary
This PR addresses an issue where the Time component scroll position was not accurate to the selected time, especially when the last option in the Time list got selected. The fix ensures that the time component scrolls to the correct selected time item. This PR will also fix the issue #4289 where the selected time is not getting auto scrolled when we use portalId
Changes Made
requestAnimationFramemethod to schedule the scroll position update, ensuring that it occurs after the next repaint, when the DOM should be updated.requestAnimationFramecallback. By checking the ref availability, the issue of rendering failures is resolved, particularly during testing with asynchronous behavior.time_format_test.test.jstests to reflect the changes made on the Time component, by addingawait waitFor(() => ...)to wait for an element an element to appear in the DOM and then make assertions.