Add token representing the height of the table with all rows visible (#…

…2307) # Pull Request ## 🤨 Rationale Resolves #1624 ## 👩‍💻 Implementation The table already knows the combined height of all the rows in the table because it uses this value to size the `table-scroll` container. Therefore, I was able to create a token that gets assigned a value within the scope of the table styling that has a value of the size of the height the table needs to show all rows. The token's name is `tableFitRowsHeight`. The table also now sets a default max-height on itself to keep a client from accidentally using `tableFitRowsHeight` and trying to render thousands of rows at once. To use the new token, a client would create their table exactly as before and add the following style: ```scss nimble-table { height: $ni-nimble-table-fit-rows-height; } ``` ## 🧪 Testing - Manually tested in storybook - Wrote chromatic tests that verify the height of the table is correct when its height is set to the new token value when: - There are 5 records, with & without grouping - There are 10 records, with & without grouping - There are 50 records, with & without grouping ## ✅ Checklist  - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed. --------- Co-authored-by: Milan Raj <[email protected]>
ni · Aug 23, 2024 · dc9d005 · dc9d005
1 parent a1809d6
commit dc9d005
Show file tree

Hide file tree

Showing 14 changed files with 241 additions and 17 deletions.
diff --git a/change/@ni-nimble-components-a5fd15e6-373b-4e3c-b562-fc2e440df26c.json b/change/@ni-nimble-components-a5fd15e6-373b-4e3c-b562-fc2e440df26c.json
@@ -0,0 +1,7 @@
+{
+  "type": "major",
+  "comment": "Create token representing the table's height with all rows visible; **Breaking change:** the table now specifies a max-height; if a different max-height is required, it needs to be configured.",
+  "packageName": "@ni/nimble-components",
+  "email": "[email protected]",
+  "dependentChangeType": "major"
+}
diff --git a/change/@ni-nimble-tokens-2857f29e-c895-406b-a302-78a6ca683d9d.json b/change/@ni-nimble-tokens-2857f29e-c895-406b-a302-78a6ca683d9d.json
@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Create token representing the table's height with all rows visible",
+  "packageName": "@ni/nimble-tokens",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/nimble-components/src/table/styles.ts b/packages/nimble-components/src/table/styles.ts
@@ -10,7 +10,10 @@ import {
     mediumPadding,
     standardPadding,
     tableRowBorderColor,
-    borderHoverColor
+    borderHoverColor,
+    controlHeight,
+    tableFitRowsHeight,
+    borderWidth
 } from '../theme-provider/design-tokens';
 import { Theme } from '../theme-provider/types';
 import { hexToRgbaCssColor } from '../utilities/style/colors';
@@ -25,6 +28,16 @@ export const styles = css`
 
     :host {
         height: 480px;
+        ${tableFitRowsHeight.cssCustomProperty}: calc(var(--ni-private-table-scroll-height) + ${controlHeight});
+        ${
+            /**
+             * Set a default maximum height for the table of 40.5 rows plus the header row so
+             * that clients don't accidentally create a table that tries to render too many rows at once.
+             * If needed, the max-height can be overridden by the client, but setting a default ensures
+             * that the max-height is considered if a larger one is needed rather than being overlooked.
+             */ ''
+        }
+        max-height: calc(${controlHeight} + (40.5 * (2 * ${borderWidth} + ${controlHeight})));
         --ni-private-column-divider-width: 2px;
         --ni-private-column-divider-padding: 3px;
     }

diff --git a/packages/nimble-components/src/table/template.ts b/packages/nimble-components/src/table/template.ts
@@ -41,11 +41,11 @@ export const template = html<Table>`
         aria-multiselectable="${x => x.ariaMultiSelectable}"
         ${children({ property: 'childItems', filter: elements() })}
     >
+        <style>:host{ --ni-private-table-scroll-height: ${x => x.virtualizer.scrollHeight}px; }</style>
         <div class="table-container ${x => (x.windowShiftKeyDown ? 'disable-select' : '')}"
             style="
             --ni-private-table-scroll-x: -${x => x.scrollX}px;
             --ni-private-table-header-container-margin-right: ${x => x.virtualizer.headerContainerMarginRight}px;
-            --ni-private-table-scroll-height: ${x => x.virtualizer.scrollHeight}px;
             --ni-private-table-row-container-top: ${x => x.virtualizer.rowContainerYOffset}px;
             --ni-private-table-row-grid-columns: ${x => (x.rowGridColumns ? x.rowGridColumns : '')};
             --ni-private-table-cursor-override: ${x => (x.layoutManager.isColumnBeingSized ? 'col-resize' : 'default')};

diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts
@@ -5,7 +5,10 @@ import { TableColumn } from '../../table-column/base';
 import { TableColumnText, tableColumnTextTag } from '../../table-column/text';
 import { TableColumnTextCellView } from '../../table-column/text/cell-view';
 import { waitForUpdatesAsync } from '../../testing/async-helpers';
-import { controlHeight } from '../../theme-provider/design-tokens';
+import {
+    controlHeight,
+    tableFitRowsHeight
+} from '../../theme-provider/design-tokens';
 import { waitForEvent } from '../../utilities/testing/component';
 import {
     type Fixture,
@@ -2338,6 +2341,75 @@ describe('Table', () => {
                 }
             );
         });
+
+        describe('styling height with tableFitRowsHeight', () => {
+            function getExpectedHeight(rowCount: number): string {
+                const rowHeight = 34;
+                const headerHeight = 32;
+                return `${rowCount * rowHeight + headerHeight}px`;
+            }
+
+            function getTableHeight(): string {
+                return getComputedStyle(element).getPropertyValue('height');
+            }
+
+            beforeEach(async () => {
+                element.style.height = `var(${tableFitRowsHeight.cssCustomProperty})`;
+                await connect();
+                await waitForUpdatesAsync();
+            });
+
+            it('has correct height before data is applied', () => {
+                const tokenValue = getTableHeight();
+                const expectedHeight = getExpectedHeight(0);
+                expect(tokenValue).toBe(expectedHeight);
+            });
+
+            it('has correct height when height changes because of setting data', async () => {
+                await element.setData(simpleTableData);
+                await waitForUpdatesAsync();
+
+                const tokenValue = getTableHeight();
+                const expectedHeight = getExpectedHeight(
+                    simpleTableData.length
+                );
+                expect(tokenValue).toBe(expectedHeight);
+            });
+
+            it('has correct height when height changes because grouping is updated', async () => {
+                await element.setData(simpleTableData);
+                await waitForUpdatesAsync();
+                column1.groupIndex = 0;
+                await waitForUpdatesAsync();
+
+                const tokenValue = getTableHeight();
+                const expectedHeight = getExpectedHeight(
+                    simpleTableData.length * 2
+                ); // Each record is grouped into its own group
+                expect(tokenValue).toBe(expectedHeight);
+            });
+
+            it('has correct height when height changes because hierarchy is collapsed', async () => {
+                const hierarchicalData = [
+                    { id: '0', parentId: undefined, stringData: 'foo' },
+                    { id: '1', parentId: '0', stringData: 'foo' },
+                    { id: '2', parentId: '0', stringData: 'foo' },
+                    { id: '3', parentId: undefined, stringData: 'foo' }
+                ];
+
+                element.idFieldName = 'id';
+                element.parentIdFieldName = 'parentId';
+                await element.setData(hierarchicalData);
+                await waitForUpdatesAsync();
+
+                pageObject.clickCollapseAllButton();
+                await waitForUpdatesAsync();
+
+                const tokenValue = getTableHeight();
+                const expectedHeight = getExpectedHeight(2); // There are only 2 top-level parents
+                expect(tokenValue).toBe(expectedHeight);
+            });
+        });
     });
 
     describe('without connection', () => {

diff --git a/packages/nimble-components/src/theme-provider/design-token-comments.ts b/packages/nimble-components/src/theme-provider/design-token-comments.ts
@@ -74,6 +74,8 @@ export const comments: { readonly [key in TokenName]: string } = {
     spinnerSmallHeight: 'Small height (16px) for a spinner component',
     spinnerMediumHeight: 'Medium height (32px) for a spinner component',
     spinnerLargeHeight: 'Large height (64px) for a spinner component',
+    tableFitRowsHeight:
+        'The height of the table when all rows are visible. It is set automatically to the correct value within the scope of each table.',
     smallDelay:
         'Elements with small transition areas, such as icons and selection controls, have short durations.',
     mediumDelay:

diff --git a/packages/nimble-components/src/theme-provider/design-token-names.ts b/packages/nimble-components/src/theme-provider/design-token-names.ts
@@ -59,6 +59,7 @@ export const tokenNames: { readonly [key in TokenName]: string } = {
     spinnerSmallHeight: 'spinner-small-height',
     spinnerMediumHeight: 'spinner-medium-height',
     spinnerLargeHeight: 'spinner-large-height',
+    tableFitRowsHeight: 'table-fit-rows-height',
     smallDelay: 'small-delay',
     mediumDelay: 'medium-delay',
     largeDelay: 'large-delay',

diff --git a/packages/nimble-components/src/theme-provider/design-tokens.ts b/packages/nimble-components/src/theme-provider/design-tokens.ts
@@ -512,6 +512,12 @@ export const spinnerLargeHeight = DesignToken.create<string>(
     styleNameFromTokenName(tokenNames.spinnerLargeHeight)
 ).withDefault('64px');
 
+// The token gets a default value of the table's default height (480px)
+// but is given a calculated value in the table styles.
+export const tableFitRowsHeight = DesignToken.create<string>(
+    styleNameFromTokenName(tokenNames.tableFitRowsHeight)
+).withDefault('480px');
+
 // Drop Shadow Tokens
 export const elevation1BoxShadow = DesignToken.create<string>(
     styleNameFromTokenName(tokenNames.elevation1BoxShadow)

diff --git a/packages/nimble-tokens/CONTRIBUTING.md b/packages/nimble-tokens/CONTRIBUTING.md
@@ -75,5 +75,5 @@ These steps require access to Adobe Illustrator and Perforce so will typically b
 4. Generate and build icon components by running `npm run build -w @ni/nimble-components`. This step will report an error at this point but is necessary to enable the next step.
 5. Add metadata for the new icons to `nimble-components/src/icon-base/tests/icon-metadata.ts`.
 6. Run `npm run build -w @ni/nimble-components` again. It should now succeed.
-7. Preview the built files by running: `npm run storybook`, and review the **Icons** story to confirm that your changes appear correctly. Inspect the icons in each **Severity** and ensure their color changes. Ensure the table is large enough to see all icons without nested scrollbars.
+7. Preview the built files by running: `npm run storybook`, and review the **Icons** story to confirm that your changes appear correctly. Inspect the icons in each **Severity** and ensure their color changes.
 8. Publish a PR with your changes. If there are any new icons, set `changeType` and `dependentChangeType` to minor in the beachball change file.
diff --git a/packages/storybook/src/docs/component-status.stories.ts b/packages/storybook/src/docs/component-status.stories.ts
@@ -7,6 +7,7 @@ import { mappingIconTag } from '../../../nimble-components/src/mapping/icon';
 import { iconCheckTag } from '../../../nimble-components/src/icons/check';
 import { iconTriangleTag } from '../../../nimble-components/src/icons/triangle';
 import { iconXmarkTag } from '../../../nimble-components/src/icons/xmark';
+import { tableFitRowsHeight } from '../../../nimble-components/src/theme-provider/design-tokens';
 import { ComponentFrameworkStatus } from './types';
 import {
     createUserSelectedThemeStory,
@@ -559,11 +560,15 @@ const metadata: Meta<TableArgs> = {
     },
     // prettier-ignore
     render: createUserSelectedThemeStory(html<TableArgs>`
+        <style class="code-hide">
+            ${tableTag} {
+                height: var(${tableFitRowsHeight.cssCustomProperty});
+                max-height: none;
+            }
+        </style>
         <${tableTag}
             ${ref('tableRef')}
             data-unused="${x => x.updateData(x)}"
-            ${/* Make the table big enough to remove vertical scrollbar */ ''}
-            style="height: calc((34px * var(--data-length)) + 32px);"
         >
             <${tableColumnAnchorTag} target="_top"
                 column-id="component-name-column"
@@ -650,10 +655,6 @@ const metadata: Meta<TableArgs> = {
                 const data = components.filter(component => (x.status === 'future'
                     ? isFuture(component)
                     : !isFuture(component)));
-                x.tableRef.style.setProperty(
-                    '--data-length',
-                    data.length.toString()
-                );
                 await x.tableRef.setData(data);
             })();
         },

diff --git a/packages/storybook/src/nimble/icon-base/icons.stories.ts b/packages/storybook/src/nimble/icon-base/icons.stories.ts
@@ -12,6 +12,7 @@ import { mappingIconTag } from '../../../../nimble-components/src/mapping/icon';
 import { tableColumnTextTag } from '../../../../nimble-components/src/table-column/text';
 import { IconSeverity } from '../../../../nimble-components/src/icon-base/types';
 import { iconMetadata } from '../../../../nimble-components/src/icon-base/tests/icon-metadata';
+import { tableFitRowsHeight } from '../../../../nimble-components/src/theme-provider/design-tokens';
 import {
     apiCategory,
     createUserSelectedThemeStory,
@@ -57,7 +58,6 @@ const updateData = (tableRef: Table<Data>): void => {
         // Safari workaround: the table element instance is made at this point
         // but doesn't seem to be upgraded to a custom element yet
         await customElements.whenDefined('nimble-table');
-        tableRef.style.setProperty('--data-length', data.length.toString());
         await tableRef.setData(data);
     })();
 };
@@ -82,10 +82,14 @@ export const icons: StoryObj<IconArgs> = {
         }
     },
     render: createUserSelectedThemeStory(html`
+        <style class="code-hide">
+            ${tableTag} {
+                height: var(${tableFitRowsHeight.cssCustomProperty});
+                max-height: none;
+            }
+        </style>
         <${tableTag}
             ${ref('tableRef')}
-            ${/* Make the table big enough to remove vertical scrollbar */ ''}
-            style="height: calc((34px * var(--data-length)) + 32px);"
             data-unused="${x => updateData(x.tableRef)}"
         >
             <${tableColumnMappingTag} field-name="tag" key-type="string" fractional-width="0.2" >

diff --git a/packages/storybook/src/nimble/table/table-fit-rows-height-matrix.stories.ts b/packages/storybook/src/nimble/table/table-fit-rows-height-matrix.stories.ts
@@ -0,0 +1,86 @@
+import type { Meta, StoryFn } from '@storybook/html';
+import { html, ViewTemplate } from '@microsoft/fast-element';
+import { tableColumnTextTag } from '../../../../nimble-components/src/table-column/text';
+import { Table, tableTag } from '../../../../nimble-components/src/table';
+import type { TableRecord } from '../../../../nimble-components/src/table/types';
+import { tableFitRowsHeight } from '../../../../nimble-components/src/theme-provider/design-tokens';
+import { createFixedThemeStory } from '../../utilities/storybook';
+import { createMatrix, sharedMatrixParameters } from '../../utilities/matrix';
+import { backgroundStates } from '../../utilities/states';
+
+interface SimpleData extends TableRecord {
+    firstName: string;
+    lastName: string;
+    favoriteColor: string;
+}
+
+const data: SimpleData[] = [];
+for (let i = 0; i < 50; i++) {
+    data.push({
+        firstName: `First Name ${i}`,
+        lastName: `Last Name ${i}`,
+        favoriteColor: `Favorite Color ${i}`
+    });
+}
+
+const groupingStates = [
+    ['Not Grouped', undefined],
+    ['Grouped', 0]
+] as const;
+type GroupingState = (typeof groupingStates)[number];
+
+const metadata: Meta = {
+    title: 'Tests/Table',
+    parameters: {
+        ...sharedMatrixParameters()
+    }
+};
+
+export default metadata;
+
+// prettier-ignore
+const component = (
+    [_groupingName, groupIndex]: GroupingState
+): ViewTemplate => html`
+    <style>
+        ${tableTag} {
+            height: var(${tableFitRowsHeight.cssCustomProperty});
+            margin-bottom: 20px;
+        }
+    </style>
+    <${tableTag}>
+        <${tableColumnTextTag} field-name="firstName" group-index="${() => groupIndex}">First Name</${tableColumnTextTag}>
+        <${tableColumnTextTag} field-name="lastName">Last Name</${tableColumnTextTag}>
+        <${tableColumnTextTag} field-name="favoriteColor">Favorite Color</${tableColumnTextTag}>
+    </${tableTag}>
+`;
+
+const playFunction = async (rowCount: number): Promise<void> => {
+    const tableData = data.slice(0, rowCount);
+    await Promise.all(
+        Array.from(document.querySelectorAll<Table>(tableTag)).map(
+            async table => {
+                await table.setData(tableData);
+            }
+        )
+    );
+};
+
+export const tableFitRowsHeightWith5Rows: StoryFn = createFixedThemeStory(
+    createMatrix(component, [groupingStates]),
+    backgroundStates[0]
+);
+
+tableFitRowsHeightWith5Rows.play = async () => playFunction(5);
+
+export const tableFitRowsHeightWith10Rows: StoryFn = createFixedThemeStory(
+    createMatrix(component, [groupingStates]),
+    backgroundStates[0]
+);
+tableFitRowsHeightWith10Rows.play = async () => playFunction(10);
+
+export const tableFitRowsHeightWith50Rows: StoryFn = createFixedThemeStory(
+    createMatrix(component, [groupingStates]),
+    backgroundStates[0]
+);
+tableFitRowsHeightWith50Rows.play = async () => playFunction(50);
diff --git a/packages/storybook/src/nimble/table/table.mdx b/packages/storybook/src/nimble/table/table.mdx
@@ -2,6 +2,7 @@ import { Controls, Canvas, Meta, Title } from '@storybook/blocks';
 import * as tableStories from './table.stories';
 import ComponentApisLink from '../../docs/component-apis-link.mdx';
 import { tableTag } from '../../../../nimble-components/src/table';
+import { tableFitRowsHeight } from '../../../../nimble-components/src/theme-provider/design-tokens';
 
 <Meta of={tableStories} />
 <Title of={tableStories} />
@@ -23,10 +24,13 @@ and the **Table Column** pages for individual table column types.
 
 ### Sizing
 
-The <Tag name={tableTag}/> should be sized explicitly or sized to fill the space
-of a parent container. The <Tag name={tableTag}/> does not currently support
-being styled with `height: auto`. The ability to auto size the table is tracked
-with [issue 1624](https://github.com/ni/nimble/issues/1624).
+The table's height can be configured to grow to fit all rows without a scrollbar
+by styling the table's height with the <code>{tableFitRowsHeight.name}</code>
+token.
+
+The table has a default maximum height that will render approximately 40 rows to
+avoid performance problems. Use caution when overriding the maximum height of
+the table because this may lead to performance issues.
 
 ### Full bleed