Skip to content
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

Table height does not account for horizontal scrollbar #2525

Merged
merged 8 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Account for horizontal scrollbar when determining value of tableFitRowsHeight token",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
4 changes: 4 additions & 0 deletions packages/nimble-components/src/table/models/virtualizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ export class Virtualizer<TData extends TableRecord = TableRecord> {
@observable
public scrollHeight = 0;

@observable
public horizontalScrollbarHeight = 0;

@observable
public isScrolling = false;

Expand Down Expand Up @@ -58,6 +61,7 @@ export class Virtualizer<TData extends TableRecord = TableRecord> {
// by the same margin so the column headers align with the corresponding rendered cells
const viewportBoundingWidth = borderBoxSize.inlineSize;
this.headerContainerMarginRight = viewportBoundingWidth - this.table.viewport.clientWidth;
this.horizontalScrollbarHeight = borderBoxSize.blockSize - this.table.viewport.clientHeight;
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion packages/nimble-components/src/table/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const styles = css`

:host {
height: 480px;
${tableFitRowsHeight.cssCustomProperty}: calc(var(--ni-private-table-scroll-height) + ${controlHeight});
${tableFitRowsHeight.cssCustomProperty}: calc(var(--ni-private-table-scroll-height) + var(--ni-private-table-horizontal-scrollbar-height) + ${controlHeight});
${
/**
* Set a default maximum height for the table of 40.5 rows plus the header row so
Expand Down
2 changes: 1 addition & 1 deletion packages/nimble-components/src/table/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ 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>
<style>:host{ --ni-private-table-scroll-height: ${x => x.virtualizer.scrollHeight}px; --ni-private-table-horizontal-scrollbar-height: ${x => x.virtualizer.horizontalScrollbarHeight}px; }</style>
<div class="table-container ${x => (x.windowShiftKeyDown ? 'disable-select' : '')}"
style="
--ni-private-table-scroll-x: -${x => x.scrollX}px;
Expand Down
22 changes: 22 additions & 0 deletions packages/nimble-components/src/table/tests/table.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2400,6 +2400,28 @@ describe('Table', () => {
expect(tokenValue).toBe(expectedHeight);
});

it('adjusts height when horizontal scrollbar is shown', async () => {
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
await element.setData(simpleTableData);
await waitForUpdatesAsync();

await pageObject.sizeTableToGivenRowWidth(100, element);
await waitForUpdatesAsync();

const tokenValue = getTableHeight();
const heightWithoutScrollbar = getExpectedHeight(
simpleTableData.length
);
// Use the `toBeGreaterThanOrEqual` comparison because in browsers with overlay scrollbars,
// the heights will match. In other browsers, the token height will be larger than the
// calculated height of the rows + header by the width of the scrollbar.
expect(parseFloat(tokenValue)).toBeGreaterThanOrEqual(
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
parseFloat(heightWithoutScrollbar)
);
expect(element.viewport.scrollHeight).toBe(
element.viewport.clientHeight
);
});

it('has correct height when height changes because of setting data', async () => {
await element.setData(simpleTableData);
await waitForUpdatesAsync();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ const groupingStates = [
] as const;
type GroupingState = (typeof groupingStates)[number];

const minColumnWidthStates = [
['Small Minimum', 100],
['Large Minimum (show horizontal scrollbar)', 500]
] as const;
type MinColumnWidthState = (typeof minColumnWidthStates)[number];

const metadata: Meta = {
title: 'Tests/Table',
parameters: {
Expand All @@ -40,18 +46,22 @@ export default metadata;

// prettier-ignore
const component = (
[_groupingName, groupIndex]: GroupingState
[_groupingName, groupIndex]: GroupingState,
[_minColumnWidthName, minColumnWidth]: MinColumnWidthState
): ViewTemplate => html`
<style>
${tableTag} {
height: var(${tableFitRowsHeight.cssCustomProperty});
/** Set a fixed width to guarantee that the large minimum column width
will cause the table to scroll horizontally. */
width: 600px;
margin-bottom: 20px;
}
</style>
<${tableTag}>
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
<${tableColumnTextTag} field-name="firstName" group-index="${() => groupIndex}">First Name</${tableColumnTextTag}>
<${tableColumnTextTag} field-name="lastName">Last Name</${tableColumnTextTag}>
<${tableColumnTextTag} field-name="favoriteColor">Favorite Color</${tableColumnTextTag}>
<${tableColumnTextTag} field-name="firstName" group-index="${() => groupIndex}" min-pixel-width="${() => minColumnWidth}">First Name</${tableColumnTextTag}>
<${tableColumnTextTag} field-name="lastName" min-pixel-width="${() => minColumnWidth}">Last Name</${tableColumnTextTag}>
<${tableColumnTextTag} field-name="favoriteColor" min-pixel-width="${() => minColumnWidth}">Favorite Color</${tableColumnTextTag}>
</${tableTag}>
`;

Expand All @@ -67,20 +77,20 @@ const playFunction = async (rowCount: number): Promise<void> => {
};

export const tableFitRowsHeightWith5Rows: StoryFn = createFixedThemeStory(
createMatrix(component, [groupingStates]),
createMatrix(component, [groupingStates, minColumnWidthStates]),
backgroundStates[0]
);

tableFitRowsHeightWith5Rows.play = async () => await playFunction(5);

export const tableFitRowsHeightWith10Rows: StoryFn = createFixedThemeStory(
createMatrix(component, [groupingStates]),
createMatrix(component, [groupingStates, minColumnWidthStates]),
backgroundStates[0]
);
tableFitRowsHeightWith10Rows.play = async () => await playFunction(10);

export const tableFitRowsHeightWith50Rows: StoryFn = createFixedThemeStory(
createMatrix(component, [groupingStates]),
createMatrix(component, [groupingStates, minColumnWidthStates]),
backgroundStates[0]
);
tableFitRowsHeightWith50Rows.play = async () => await playFunction(50);