Conversation
Replace the requestAnimationFrame-based scroll handler with synchronous reads and remove the header's translateX/offsetX approach. Instead, set the header element's scrollLeft directly from the body's scroll event, eliminating the visual lag between header and body during horizontal scrolling. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add a second datatable to the scrolling demo that generates columns dynamically via @for over a signal-based config. Each column uses custom header and cell templates with @switch for varied rendering (text, emphasis, progress bar, badge, link), reproducing the real-world pattern that triggered the horizontal scroll flicker. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary of ChangesHello @drdreo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a horizontal scroll flicker issue in the datatable component. The solution optimizes scroll synchronization between the table body and header by removing a Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the horizontal scroll flicker by synchronizing the header and body scroll positions via direct DOM manipulation. The removal of requestAnimationFrame and the CSS translateX transform in favor of directly setting scrollLeft is a direct and appropriate fix for the described lag. The changes are well-implemented, and the addition of a more complex demo case is a great way to validate the solution. I've included a couple of suggestions for minor improvements related to Angular best practices.
| // Sync header scroll position directly via DOM to avoid Angular CD lag | ||
| const headerEl = this._headerElement()?.nativeElement; | ||
| if (headerEl) { | ||
| headerEl.scrollLeft = event.offsetX; |
There was a problem hiding this comment.
For better platform abstraction and to adhere to Angular best practices, it's recommended to use Renderer2 for direct DOM manipulation instead of accessing nativeElement properties directly. This helps ensure your component can work in environments without direct DOM access, like server-side rendering.
You would need to inject Renderer2:
import { Renderer2 } from '@angular/core';
// ...
private renderer = inject(Renderer2);And then use it here.
this.renderer.setProperty(headerEl, 'scrollLeft', event.offsetX);| @switch (col.type) { | ||
| @case ('progress') { | ||
| <div style="display: flex; align-items: center; gap: 8px;"> | ||
| <div | ||
| style="background: #4caf50; height: 8px; border-radius: 4px;" | ||
| [style.width.px]="value" | ||
| ></div> | ||
| <span>{{ value }}</span> | ||
| </div> | ||
| } | ||
| @case ('badge') { | ||
| <span | ||
| style="background: #e3f2fd; padding: 2px 8px; border-radius: 12px; font-size: 12px;" | ||
| > | ||
| {{ value }} | ||
| </span> | ||
| } | ||
| @case ('link') { | ||
| <span style="color: #1976d2; text-decoration: underline; cursor: pointer;">{{ | ||
| value | ||
| }}</span> | ||
| } | ||
| @case ('emphasis') { | ||
| <em>{{ value }}</em> | ||
| } | ||
| @default { | ||
| {{ value }} | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is a demo component, there are multiple inline styles being used. For better maintainability and separation of concerns, consider moving these styles to a dedicated stylesheet and using CSS classes. This would make the template cleaner and the styles easier to manage. For example, you could add a styleUrls property to your @Component decorator and define classes for progress, badge, link, etc.
2 participants
What kind of change does this PR introduce?
What is the current behavior? (You can also link to an open issue here)
During horizontal scrolling, the header visually lags behind the body. It briefly resets to the left before snapping back into position, causing a noticeable flicker. This is caused by the requestAnimationFrame wrapper in the scroll handler and the header using a CSS translate transform that goes through Angular change detection, rather than scrolling in sync with the body.
This is easily verifiable by pulling the demo page changes (
scrolling.component.ts) and scrolling horizontally, one will observe a clear stutter / flicker. I have only observed this with a more real-world setup: custom templates, dynamically renderd columns, etc.What is the new behavior?
Header and body scroll in sync via direct DOM manipulation, bypassing Angular CD entirely. The translateX/offsetX approach on the header is removed. I added a demo with dynamic columns and custom templates to the scrolling example to validate the fix under real-world conditions.
Does this PR introduce a breaking change? (check one with "x")