feat(table): extrai o header de dentro cdk-virtual-scroll#2773
feat(table): extrai o header de dentro cdk-virtual-scroll#2773marioAntonioTotvs wants to merge 1 commit intomasterfrom
Conversation
Tabela com muitos itens ao realizar o scroll acontece flickering (piscar) em toda tabela. Extrai o <thead> de dentro do 'cdk-virtual-scroll-viewport' mantendo a visualização da tag ao realizar o scroll. Feat: DTHFUI-11105
b59b151 to
8e92e80
Compare
| private scrollEvent$: Observable<any>; | ||
| private subscriptionScrollEvent: Subscription; | ||
| private subscriptionService: Subscription = new Subscription(); | ||
| private columnWidths: Array<string> = []; |
There was a problem hiding this comment.
Parece que essa variável foi declarada mas não está sendo usada. Podemos removê-la?
|
|
||
| // Agenda sincronização quando virtual scroll está ativo mas não há larguras computadas | ||
| if ( | ||
| this.virtualScroll && |
There was a problem hiding this comment.
Esse if possui várias condições que podem dificultar o entendimento do código... teria como movê-lo para um método privado com um nome intuitivo do que está sendo analisado aqui? Exemplo (de acordo com o comentário acima): shouldScheduleColumnSync, shouldScheduleSyncWithoutWidths, schedSyncVirtualScrollWithoutWidths...
|
|
||
| if (headerTable) { | ||
| const headerCells = headerTable.querySelectorAll('thead th'); | ||
| for (let i = 0; i < headerCells.length; i++) { |
There was a problem hiding this comment.
Essa parte parece fazer a mesma coisa que o trecho abaixo, mudando apenas o elemento e o que deve ser passado no querySelectorAll. Seria possível extrair para uma função que recebe como parâmetro o elemento e o query selector e executar a lógica? Assim a função poderia ser chamadas nesses dois pontos, diminuindo a duplicidade de lógica.
|
|
||
| if (!this.scrollSyncListener) { | ||
| this.scrollSyncListener = this.renderer.listen(viewportEl, 'scroll', () => { | ||
| if (this.headerScrollContainer?.nativeElement) { |
There was a problem hiding this comment.
Essa lógica está igual a da linha 1084. Seria possível levar para uma função e evitar duplicidade de lógica? A nova função poderia receber o elemento como parâmetro, permitindo que o if fique dentro dela tb.
|
|
||
| const viewportEl = this.tableVirtualScroll.nativeElement; | ||
|
|
||
| const contentWrapper = viewportEl.querySelector('.cdk-virtual-scroll-content-wrapper'); |
There was a problem hiding this comment.
Será que não seria interessante adicionar esse seletor em uma const?
Pois se um dia precisarmos utilizá-lo em outro ponto, não precisamos reescrevê-lo por inteiro, apenas chamar pela const e, se também algum dia ele for alterado, de vez alterarmos nos pontos em que ele é chamado, poderíamos alterar somente na atribuição da const.
|
|
||
| // O scroll horizontal real acontece no container .po-table-container-fixed-inner (pai do viewport). | ||
| // Precisamos sincronizar o scrollLeft do header com esse container também. | ||
| const fixedInnerContainer = viewportEl.closest('.po-table-container-fixed-inner'); |
There was a problem hiding this comment.
Será que não seria interessante adicionar esse seletor em uma const?
Pois se um dia precisarmos utilizá-lo em outro ponto, não precisamos reescrevê-lo por inteiro, apenas chamar pela const e, se também algum dia ele for alterado, de vez alterarmos nos pontos em que ele é chamado, poderíamos alterar somente na atribuição da const.
| > | ||
| <table | ||
| #bodyTable | ||
| class="po-table po-table-virtual-scroll" |
There was a problem hiding this comment.
Não achei a declaração dessa classe (po-table-virtual-scroll) css no po-style e nem no po-angular. Está correto? Onde encontro ela?
|
|
||
| // O header precisa de overflow: hidden para que scrollLeft funcione via JS. | ||
| // Sem isso, o elemento não cria contexto de scroll e scrollLeft fica sempre em 0. | ||
| if (this.headerScrollContainer?.nativeElement) { |
There was a problem hiding this comment.
Essa função está agregando mais de uma responsabilidade? Se sim, será que não seria interessante cada bloco ser uma função distinta?
| const bodyTable = this.bodyTableElement.nativeElement; | ||
|
|
||
| // Seleciona apenas cells das mainColumns (ignora selectable, action, etc.) | ||
| const headerCells = headerTable.querySelectorAll('thead th.po-table-header-ellipsis'); |
There was a problem hiding this comment.
Será que não seria interessante esse seletor ('thead th.po-table-header-ellipsis') virar uma const tb?
| this.measureCellWidths(headerTable, headerCells, count, maxColumnWidths); | ||
|
|
||
| // Fase 3: Armazenar larguras computadas e aplicar via Renderer2 | ||
| this.computedColumnWidths = maxColumnWidths.map(w => `${w}px`); |
There was a problem hiding this comment.
Entendo que as responsabilidade dessa função estão sendo explicadas em comentários, mas será que não seria interessante dividi-las em funções distintas para melhor legibilidade da função principal/chamadora?
deboraconstantino
left a comment
There was a problem hiding this comment.
Testes de code review com o Devin Review. Link da sessão: https://totvs.devinenterprise.com/sessions/3ae632f28d434ea0bf0478c7d4c92c0e
| @@ -718,7 +784,7 @@ export class PoTableComponent extends PoTableBaseComponent implements AfterViewI | |||
| this.heightTableContainer = height ? height - this.getHeightTableFooter() : undefined; | |||
| this.heightTableVirtual = this.heightTableContainer ? this.heightTableContainer - this.itemSize : undefined; | |||
There was a problem hiding this comment.
🔴 Virtual scroll viewport height calculated using body row height instead of actual header height
The heightTableVirtual is computed as heightTableContainer - itemSize (po-table.component.ts:785), where itemSize is the body row height from PO_TABLE_ROW_HEIGHT_BY_SPACING. However, in the new split-table layout, this value is used for the cdk-virtual-scroll-viewport height (po-table.component.html:697), while the header table sits above the viewport inside a wrapper of height heightTableContainer (po-table.component.html:537). If the actual header height differs from itemSize (e.g., due to sort icons, different padding, multi-line headers, or different spacing configuration between header/body), the viewport will either overflow the wrapper (header taller than itemSize) or leave a visible gap (header shorter). The old code didn't have this issue because the header was inside the viewport using position: sticky.
Prompt for agents
In projects/ui/src/lib/components/po-table/po-table.component.ts, the heightTableVirtual calculation at line 785 uses itemSize (body row height) to subtract from heightTableContainer. However, the actual header height may differ from itemSize. Instead of subtracting itemSize, the code should dynamically measure the actual rendered header height (e.g., from this.headerScrollContainer?.nativeElement?.offsetHeight or this.headerTableElement?.nativeElement?.offsetHeight) and subtract that. This should be recalculated whenever the header height changes. For example, change line 785 to something like:
const headerHeight = this.headerScrollContainer?.nativeElement?.offsetHeight || this.itemSize;
this.heightTableVirtual = this.heightTableContainer ? this.heightTableContainer - headerHeight : undefined;
Also consider recalculating this in ngAfterViewChecked when the header element is available, since during the initial calculateHeightTableContainer call the header may not yet be rendered.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Re-sincroniza larguras após Angular renderizar a nova ordem | ||
| setTimeout(() => this.syncColumnWidths()); |
There was a problem hiding this comment.
🟡 Double syncColumnWidths scheduling in drop() when hideColumnsManager is false
In the drop method (po-table.component.ts:725-750), when hideColumnsManager === false, the code calls onVisibleColumnsChange() at line 745, which internally schedules setTimeout(() => this.syncColumnWidths()) when virtualScroll is true (po-table.component.ts:643). Then at line 749, drop also unconditionally schedules another setTimeout(() => this.syncColumnWidths()). This causes syncColumnWidths to execute twice in quick succession, performing redundant DOM measurements (including getBoundingClientRect calls and temporary style manipulation via max-content). Similarly, clearColumnWidths is called twice synchronously (line 727 and inside onVisibleColumnsChange at line 637).
| // Re-sincroniza larguras após Angular renderizar a nova ordem | |
| setTimeout(() => this.syncColumnWidths()); | |
| // Re-sincroniza larguras após Angular renderizar a nova ordem | |
| if (this.hideColumnsManager) { | |
| setTimeout(() => this.syncColumnWidths()); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| [ngStyle]="{ | ||
| 'width': | ||
| height > 0 && !virtualScroll ? (!hasItems ? '100%' : applyFixedColumns() ? column.width : 'auto') : '' | ||
| height > 0 && !virtualScroll ? (!hasItems ? '100%' : applyFixedColumns() ? column.width : null) : null |
There was a problem hiding this comment.
📝 Info: Semantic change from 'auto' to null in ngStyle width for non-virtual-scroll table
Lines 259 and 299 change the [ngStyle] width value from 'auto' / '' to null / null. When height > 0 && !virtualScroll && hasItems && !applyFixedColumns(), the old code set width: auto while the new code removes the width property entirely (via null). Since auto is the CSS default for width, this is functionally equivalent in most scenarios. However, if any CSS rule explicitly sets a different width on these <th> elements, auto would override it while removing the style would not. This appears intentional but is a subtle behavioral change worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ( | ||
| this.virtualScroll && | ||
| this.hasItems && | ||
| !this.applyFixedColumns() && | ||
| this.computedColumnWidths.length === 0 && | ||
| this.bodyTableElement?.nativeElement?.querySelector('tbody tr') && | ||
| !this.syncScheduled | ||
| ) { | ||
| this.syncScheduled = true; | ||
| setTimeout(() => { | ||
| this.syncColumnWidths(); | ||
| this.syncScheduled = false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
📝 Info: ngAfterViewChecked performs DOM query on every change detection cycle
At po-table.component.ts:334, this.bodyTableElement?.nativeElement?.querySelector('tbody tr') executes a DOM query inside ngAfterViewChecked, which runs on every change detection cycle. While the condition chain (virtualScroll, hasItems, !applyFixedColumns, etc.) gates this, applyFixedColumns() itself calls this.columns.some(column => !column.width) which also runs every cycle. Combined with the same applyFixedColumns() calls in the template (called per-column per-row in the virtual scroll body), this could have measurable performance impact with large datasets. Consider caching the result of applyFixedColumns() per change detection cycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private setupColumnWidthSync(): void { | ||
| if (!this.virtualScroll) return; | ||
|
|
||
| this.resizeObserver = new ResizeObserver(() => { | ||
| this.syncColumnWidths(); | ||
| }); | ||
|
|
||
| const viewportEl = this.tableVirtualScroll?.nativeElement; | ||
| if (viewportEl) { | ||
| this.resizeObserver.observe(viewportEl); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: setupColumnWidthSync only runs once at init and won't activate if virtualScroll is enabled later
The setupColumnWidthSync() method is called once in ngAfterViewInit() (line 319) and returns early if this.virtualScroll is false (line 1098). If virtualScroll is toggled to true after initialization (e.g., by dynamically setting p-height and p-virtual-scroll), the ResizeObserver will never be created. The ngAfterViewChecked fallback mechanism (syncScheduled flag) partially compensates by scheduling syncs when needed, but the ResizeObserver provides more responsive updates. This may be acceptable given the component's usage patterns but is worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onVisibleColumnsChange(columns: Array<PoTableColumn>) { | ||
| this.clearColumnWidths(); | ||
| this.columns = columns; | ||
| this.changeDetector.detectChanges(); | ||
| this.changeDetector.markForCheck(); | ||
|
|
||
| // Re-sincroniza larguras após Angular renderizar a nova configuração de colunas | ||
| if (this.virtualScroll) { | ||
| setTimeout(() => this.syncColumnWidths()); | ||
| } |
There was a problem hiding this comment.
📝 Info: Change from detectChanges to markForCheck in onVisibleColumnsChange is intentional but changes timing
The onVisibleColumnsChange method changed from this.changeDetector.detectChanges() (synchronous) to this.changeDetector.markForCheck() (asynchronous). Conversely, calculateHeightTableContainer changed from markForCheck() to detectChanges(). The test updates confirm these are intentional. The markForCheck change in onVisibleColumnsChange means the DOM won't be updated synchronously after column changes — but since this is called from event handlers, Angular's zone will trigger change detection shortly after, making this safe in practice.
Was this helpful? React with 👍 or 👎 to provide feedback.
Tabela com muitos itens ao realizar o scroll acontece flickering (piscar) em toda tabela. Extrai o de dentro do 'cdk-virtual-scroll-viewport' mantendo a visualização da tag ao realizar o scroll.
Feat: DTHFUI-11105
PO-TABLE
DTHFUI-11105
PR Checklist [Revisor]
Qual o comportamento atual?
Tabela com muitos itens ao realizar o scroll acontece flickering (piscar) em toda tabela
Qual o novo comportamento?
Extrai o de dentro do 'cdk-virtual-scroll-viewport' mantendo a visualização da tag ao realizar o scroll.
Simulação
app.zip