-
-
Notifications
You must be signed in to change notification settings - Fork 620
chore(webui): Migrate from JS to CSS for eval results scroll effects #4995
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
TestGru AssignmentSummary
Files
Tip You can |
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes introduce a polyfill for the CSS Scroll Timeline and View Timeline APIs by adding a new JavaScript file and loading it in the main HTML file. The polyfill enables scroll-driven animations in browsers without native support. CSS in the results table and view components is refactored to use scroll timelines and CSS custom properties for animating header collapse, replacing previous manual class toggling and scroll event handling. Corresponding TypeScript components have scroll-related props, state, and event handlers removed, shifting all header collapse logic to CSS and scroll timelines. The configuration file is updated to exclude the polyfill script directory from certain processing. Estimated code review effort4 (~90 minutes) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Generated 7 tests - 7 passed (96c807d) View tests ↗Test Summary
ResultsAll Tusk tests are passing for the CSS scroll-timeline migration. The tests verify that the new implementation correctly renders the required DOM elements ( Key points:
View check history
Was Tusk helpful? Give feedback by reacting with 👍 or 👎 |
JS-based scrolling contains a broken edge case where shifting the scroll container height (by resizing the header) results in the scroll handler being called recursively until the user changes the scroll y position.
This PR migrates to CSS-based scrolling effects using a scroll timeline. Scroll timelines are a new-ish CSS feature, with availability in Chrome and Edge, beta support in Safari, and feature-flagged support in Firefox, so this PR includes the polyfill recommended by Chrome for Developers.
Unfortunately I was not able to get scroll animations working on ancestor (of the scrollable element) components in Firefox and Safari, and thus have disabled hiding the eval header on scroll, which I believe is preferable to have behavior differ by browser. This should be considered a temporary regression as the header requires an informatic-heirarchy update and is likely to soon change regardless.