From 4cfd1a6c65d93d8377f1c1df79ca6ff6746cd2f5 Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Tue, 10 Sep 2024 18:26:31 -0500 Subject: [PATCH] Table: Move row height calculation from virtualizer, monitor for token changes (#2388) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale #2212 ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation - Moved row height calculation from virtualizer to table class (new `internal` property there for row height) - Added subscribers to listen for changes to the `controlHeight` and `borderWidth` design tokens, which are used for row height calculation ## ๐Ÿงช Testing - No table visual diffs with this change - Added autotests to ensure that rowHeight/pageSize are recomputed when the `controlHeight` and `borderWidth` tokens change - Manual testing of PageUp/PageDown/Home/End key behavior after tokens change ## โœ… Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. --- ...-49250ef8-26bc-404b-bba7-342b1f3b6a8e.json | 7 +++ packages/nimble-components/src/table/index.ts | 37 ++++++++++++++++ .../src/table/models/virtualizer.ts | 20 ++++----- .../src/table/tests/table.spec.ts | 44 +++++++++++++++++++ packages/storybook/.storybook/preview.js | 2 +- 5 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 change/@ni-nimble-components-49250ef8-26bc-404b-bba7-342b1f3b6a8e.json diff --git a/change/@ni-nimble-components-49250ef8-26bc-404b-bba7-342b1f3b6a8e.json b/change/@ni-nimble-components-49250ef8-26bc-404b-bba7-342b1f3b6a8e.json new file mode 100644 index 0000000000..27c5a7b212 --- /dev/null +++ b/change/@ni-nimble-components-49250ef8-26bc-404b-bba7-342b1f3b6a8e.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Table: move row height computation out of virtualizer; row height recomputed if dependent tokens change.", + "packageName": "@ni/nimble-components", + "email": "20709258+msmithNI@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/nimble-components/src/table/index.ts b/packages/nimble-components/src/table/index.ts index 067ec26158..1c57530428 100644 --- a/packages/nimble-components/src/table/index.ts +++ b/packages/nimble-components/src/table/index.ts @@ -9,6 +9,7 @@ import { import { Checkbox, DesignSystem, + DesignTokenSubscriber, FoundationElement } from '@microsoft/fast-foundation'; import { @@ -65,6 +66,7 @@ import { waitUntilCustomElementsDefinedAsync } from '../utilities/wait-until-cus import { ColumnValidator } from '../table-column/base/models/column-validator'; import { uniquifySlotNameForColumnId } from './models/utilities'; import { KeyboardNavigationManager } from './models/keyboard-navigation-manager'; +import { borderWidth, controlHeight } from '../theme-provider/design-tokens'; declare global { interface HTMLElementTagNameMap { @@ -251,6 +253,13 @@ export class Table< @observable public windowShiftKeyDown = false; + /** + * @internal + */ + public get rowHeight(): number { + return this._rowHeight; + } + private readonly table: TanStackTable>; private options: TanStackTableOptionsResolved>; private readonly tableValidator = new TableValidator(); @@ -260,6 +269,7 @@ export class Table< private readonly expansionManager: ExpansionManager; private columnNotifiers: Notifier[] = []; private readonly layoutManagerNotifier: Notifier; + private _rowHeight = 0; private isInitialized = false; // Programmatically updating the selection state of a checkbox fires the 'change' event. // Therefore, selection change events that occur due to programmatically updating @@ -273,6 +283,22 @@ export class Table< { recordId: string, uniqueSlot: string } > = new Map(); + private readonly borderWidthSubscriber: DesignTokenSubscriber< + typeof borderWidth + > = { + handleChange: () => { + this.updateRowHeight(); + } + }; + + private readonly controlHeightSubscriber: DesignTokenSubscriber< + typeof controlHeight + > = { + handleChange: () => { + this.updateRowHeight(); + } + }; + private actionMenuSlots: string[] = []; public constructor() { @@ -362,6 +388,7 @@ export class Table< public override connectedCallback(): void { super.connectedCallback(); this.initialize(); + this.updateRowHeight(); this.virtualizer.connect(); this.viewport.addEventListener('scroll', this.onViewPortScroll, { passive: true @@ -370,6 +397,8 @@ export class Table< window.addEventListener('keydown', this.onKeyDown); window.addEventListener('keyup', this.onKeyUp); window.addEventListener('blur', this.onBlur); + borderWidth.subscribe(this.borderWidthSubscriber, this); + controlHeight.subscribe(this.controlHeightSubscriber, this); } public override disconnectedCallback(): void { @@ -380,6 +409,8 @@ export class Table< window.removeEventListener('keydown', this.onKeyDown); window.removeEventListener('keyup', this.onKeyUp); window.removeEventListener('blur', this.onBlur); + borderWidth.unsubscribe(this.borderWidthSubscriber); + controlHeight.unsubscribe(this.controlHeightSubscriber); } public checkValidity(): boolean { @@ -1332,6 +1363,12 @@ export class Table< return tanstackSelectionState; } + + private updateRowHeight(): void { + this._rowHeight = parseFloat(controlHeight.getValueFor(this)) + + 2 * parseFloat(borderWidth.getValueFor(this)); + this.virtualizer?.updateRowHeight(); + } } const nimbleTable = Table.compose({ diff --git a/packages/nimble-components/src/table/models/virtualizer.ts b/packages/nimble-components/src/table/models/virtualizer.ts index 739962822e..31f1314da1 100644 --- a/packages/nimble-components/src/table/models/virtualizer.ts +++ b/packages/nimble-components/src/table/models/virtualizer.ts @@ -9,7 +9,6 @@ import { VirtualItem, ScrollToOptions } from '@tanstack/virtual-core'; -import { borderWidth, controlHeight } from '../../theme-provider/design-tokens'; import type { Table } from '..'; import type { TableNode, TableRecord } from '../types'; @@ -38,13 +37,6 @@ export class Virtualizer { return this._pageSize; } - private get rowHeight(): number { - return ( - parseFloat(controlHeight.getValueFor(this.table)) - + 2 * parseFloat(borderWidth.getValueFor(this.table)) - ); - } - private readonly table: Table; private readonly tanStackTable: TanStackTable>; private readonly viewportResizeObserver: ResizeObserver; @@ -93,6 +85,14 @@ export class Virtualizer { this.virtualizer?.scrollToIndex(index, options); } + public updateRowHeight(): void { + this.updatePageSize(); + if (this.virtualizer) { + this.updateVirtualizer(); + this.virtualizer.measure(); + } + } + private updateVirtualizer(): void { const options = this.createVirtualizerOptions(); if (this.virtualizer) { @@ -108,7 +108,7 @@ export class Virtualizer { HTMLElement, HTMLElement > { - const rowHeight = this.rowHeight; + const rowHeight = this.table.rowHeight; return { count: this.tanStackTable.getRowModel().rows.length, getScrollElement: () => { @@ -154,7 +154,7 @@ export class Virtualizer { private updatePageSize(): void { this._pageSize = Math.round( - this.table.viewport.clientHeight / this.rowHeight + this.table.viewport.clientHeight / this.table.rowHeight ); } } diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts index c4f66c64ff..38331162fa 100644 --- a/packages/nimble-components/src/table/tests/table.spec.ts +++ b/packages/nimble-components/src/table/tests/table.spec.ts @@ -6,6 +6,7 @@ import { TableColumnText, tableColumnTextTag } from '../../table-column/text'; import { TableColumnTextCellView } from '../../table-column/text/cell-view'; import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { + borderWidth, controlHeight, tableFitRowsHeight } from '../../theme-provider/design-tokens'; @@ -2442,6 +2443,49 @@ describe('Table', () => { expect(tokenValue).toBe(expectedHeight); }); }); + + describe('row height calculation', () => { + it('is computed with a positive value', async () => { + await connect(); + await waitForUpdatesAsync(); + + expect(element.rowHeight).toBeGreaterThan(0); + }); + + it('rowHeight and pageSize are recomputed when the borderWidth design token changes', async () => { + await connect(); + await waitForUpdatesAsync(); + + const initialRowHeight = element.rowHeight; + const initialPageSize = element.virtualizer.pageSize; + const initialBorderWidth = borderWidth.getValueFor(element); + const newBorderWidth = parseFloat(initialBorderWidth) + 8; + borderWidth.setValueFor(element, `${newBorderWidth}px`); + await waitForUpdatesAsync(); + + expect(element.rowHeight).toBeGreaterThan(initialRowHeight); + expect(element.virtualizer.pageSize).toBeLessThan( + initialPageSize + ); + }); + + it('rowHeight and pageSize are recomputed when the controlHeight design token changes', async () => { + await connect(); + await waitForUpdatesAsync(); + + const initialRowHeight = element.rowHeight; + const initialPageSize = element.virtualizer.pageSize; + const initialControlHeight = controlHeight.getValueFor(element); + const newControlHeight = parseFloat(initialControlHeight) * 2; + controlHeight.setValueFor(element, `${newControlHeight}px`); + await waitForUpdatesAsync(); + + expect(element.rowHeight).toBeGreaterThan(initialRowHeight); + expect(element.virtualizer.pageSize).toBeLessThan( + initialPageSize + ); + }); + }); }); describe('without connection', () => { diff --git a/packages/storybook/.storybook/preview.js b/packages/storybook/.storybook/preview.js index 2942013cdb..4f1c685ee7 100644 --- a/packages/storybook/.storybook/preview.js +++ b/packages/storybook/.storybook/preview.js @@ -105,5 +105,5 @@ configureActions({ depth: 1 }); -// Update the GUID on this line to trigger a turbosnap full rebuild: c250c9b3-0bdf-4f27-a923-672573a79aaa +// Update the GUID on this line to trigger a turbosnap full rebuild: 35fca962-ed23-4476-979e-094c9a0b48f3 // See https://www.chromatic.com/docs/turbosnap/#full-rebuilds