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

[EuiDataGrid] Various row height fixes #8025

Merged
merged 11 commits into from
Sep 19, 2024
Merged
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions packages/eui/changelogs/upcoming/8025.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
**Bug fixes**

- Fixed several `EuiDataGrid` row height bugs:
- Fixed row heights not recalculating when `rowHeightOptions.lineHeight`, `gridStyles.fontSize`, or `gridStyles.cellPadding` changed
- Fixed incorrect height calculations for `rowHeightOptions.rowHeights` with `lineCount`s
- Fixed control column content to align better with multi-line row heights, as well as custom line-heights
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ exports[`EuiDataGrid rendering renders additional toolbar controls 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="A"
>
A
Expand Down Expand Up @@ -653,7 +653,7 @@ exports[`EuiDataGrid rendering renders additional toolbar controls 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="B"
>
B
Expand Down Expand Up @@ -989,7 +989,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="A"
>
A
Expand Down Expand Up @@ -1031,7 +1031,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="B"
>
B
Expand Down Expand Up @@ -1085,7 +1085,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
0
Expand Down Expand Up @@ -1142,7 +1142,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
0
Expand All @@ -1161,7 +1161,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
1
Expand Down Expand Up @@ -1218,7 +1218,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
1
Expand All @@ -1237,7 +1237,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
2
Expand Down Expand Up @@ -1294,7 +1294,7 @@ exports[`EuiDataGrid rendering renders control columns 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-defaultHeight"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight emotion-euiDataGridRowCell__content-controlColumn-autoHeight"
data-datagrid-cellcontent="true"
>
2
Expand Down Expand Up @@ -1478,7 +1478,7 @@ exports[`EuiDataGrid rendering renders custom column headers 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="A"
>
Column A
Expand Down Expand Up @@ -1520,7 +1520,7 @@ exports[`EuiDataGrid rendering renders custom column headers 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="B"
>
<div>
Expand Down Expand Up @@ -1840,7 +1840,7 @@ exports[`EuiDataGrid rendering renders with common and div attributes 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="A"
>
A
Expand Down Expand Up @@ -1882,7 +1882,7 @@ exports[`EuiDataGrid rendering renders with common and div attributes 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="B"
>
B
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="columnA"
>
columnA
Expand Down Expand Up @@ -68,7 +68,7 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="columnB"
>
columnB
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ exports[`EuiDataGridBodyVirtualized renders 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="columnA"
>
columnA
Expand Down Expand Up @@ -72,7 +72,7 @@ exports[`EuiDataGridBodyVirtualized renders 1`] = `
data-test-subj="dataGridColumnResizer"
/>
<div
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content-left"
class="euiDataGridHeaderCell__content emotion-euiDataGridHeaderCell__content"
title="columnB"
>
columnB
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export const euiDataGridCellOutlineSelectors = (parentSelector = '&') => {
};

export const euiDataGridRowCellStyles = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;
const cellOutline = euiDataGridCellOutlineStyles(euiThemeContext);
const { outline: outlineSelectors } = euiDataGridCellOutlineSelectors();

Expand Down Expand Up @@ -146,17 +147,45 @@ export const euiDataGridRowCellStyles = (euiThemeContext: UseEuiTheme) => {
euiDataGridRowCell__content: css`
overflow: hidden;
`,
controlColumn: css`
${logicalCSS('max-height', '100%')}
display: flex;
align-items: center;
`,
autoHeight: css`
${logicalCSS('height', 'auto')}
`,
defaultHeight: css`
${logicalCSS('height', '100%')}
`,
// Control columns should be vertically centered with the *first line* of text
// for both single and multi-line heights (see https://github.com/elastic/eui/issues/7897)
controlColumn: css`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we align the content horizontally as well?
Currently the controls in the story examples are not centered for cellPadding="s | l"

Screenshot 2024-09-18 at 15 59 42

Screenshot 2024-09-18 at 15 44 40

Screenshot 2024-09-18 at 15 44 48

Copy link
Member Author

@cee-chen cee-chen Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this too and I'm debating a separate PR for that where controlColumns.width does not include the cell padding, so that it scales with different densities and this off-centeredness doesn't occur.

Center alignment would solve the issue for the compact density, but not the expanded density, since technically the width provided is too small for both the expanded cell padding + the checkbox width.

Either way I'd like to address that separately from this PR, since that has more to do with column widths and less to do with row heights, and changing how the width API works for control columns would technically be a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context! I'm fine with addressing it in a separate PR then 👍

display: inline-flex;
align-items: start;
gap: ${euiTheme.size.xxs}; /* EuiButtonIcons */

/* FF and webkit browsers have more centered vertical alignment behind undocumented browser prefixes */
vertical-align: -webkit-baseline-middle;
vertical-align: -moz-middle-with-baseline;

/* Compact sizing affordance for EuiButtonIcons */
.euiDataGrid--fontSizeSmall
&:where(.euiDataGridRowCell__content--defaultHeight) {
${logicalCSS('height', '100%')}
align-items: center;
}

/* Line up single EuiCheckboxes a bit better (insert Peter Griffin blinds gif) */
.euiCheckbox:not(:has(label)) {
display: inline;

.euiCheckbox__square {
display: inline-flex;
vertical-align: text-bottom;

/* sub alignment looks better on Firefox, text-bottom looks better on webkit 💀 */
@supports (vertical-align: -moz-middle-with-baseline) {
vertical-align: sub;
}
}
}
`,
},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ describe('EuiDataGridCell', () => {
),
popoverContext: mockPopoverContext,
rowHeightUtils: mockRowHeightUtils,
gridStyles: {},
};

beforeEach(() => jest.clearAllMocks());
Expand Down Expand Up @@ -202,6 +203,12 @@ describe('EuiDataGridCell', () => {
it('rowHeightsOptions', () => {
component.setProps({ rowHeightsOptions: { defaultHeight: 'auto' } });
});
it('gridStyles.fontSize', () => {
component.setProps({ gridStyles: { fontSize: 's' } });
});
it('gridStyles.cellPadding', () => {
component.setProps({ gridStyles: { cellPadding: 'l' } });
});
it('renderCellValue', () => {
component.setProps({ renderCellValue: () => <div>test</div> });
});
Expand Down Expand Up @@ -254,7 +261,10 @@ describe('EuiDataGridCell', () => {
});

it('should not update for prop/state changes not specified above', () => {
component.setProps({ className: 'test' });
component.setProps({
className: 'test',
gridStyles: { header: 'underline' },
});
expect(shouldComponentUpdate).toHaveReturnedWith(false);
});
});
Expand Down Expand Up @@ -625,7 +635,7 @@ describe('EuiDataGridCell', () => {
callMethod(component);
expect(
mockRowHeightUtils.calculateHeightForLineCount
).toHaveBeenCalledWith(expect.any(HTMLElement), 3, false);
).toHaveBeenCalledWith(expect.any(HTMLElement), 3);
expect(setRowHeight).toHaveBeenCalled();
});
});
Expand All @@ -647,25 +657,76 @@ describe('EuiDataGridCell', () => {
callMethod(component);
expect(
mockRowHeightUtils.calculateHeightForLineCount
).toHaveBeenCalledWith(expect.any(HTMLElement), 10, true);
).toHaveBeenCalledWith(expect.any(HTMLElement), 10);
expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalled();
expect(setRowHeight).not.toHaveBeenCalled();
});

it('recalculates when the override for the row changes', () => {
const component = mount(
<EuiDataGridCell {...requiredProps} setRowHeight={setRowHeight} />
);

component.setProps({
rowHeightsOptions: {
rowHeights: {
0: { lineCount: 2 },
},
},
});
expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalledTimes(1);

// Handle row index changes as well
component.setProps({
rowHeightsOptions: {
rowHeights: {
0: { lineCount: 2 },
2: { lineCount: 4 },
},
},
rowIndex: 2,
});
expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalledTimes(2);

expect(setRowHeight).not.toHaveBeenCalled();
});
});

it('recalculates when rowHeightsOptions.defaultHeight.lineCount changes', () => {
it('recalculates when props that affect row/line height change', () => {
const component = mount(
<EuiDataGridCell
{...requiredProps}
rowHeightsOptions={{ defaultHeight: { lineCount: 7 } }}
rowHeightsOptions={{ defaultHeight: { lineCount: 4 } }}
setRowHeight={setRowHeight}
/>
);
component.setProps({
rowHeightsOptions: { defaultHeight: { lineCount: 2 } },
});
expect(setRowHeight).toHaveBeenCalledTimes(1);

// Other props that can affect row heights

const rowHeightsOptionsWithLineHeight = {
defaultHeight: { lineCount: 2 },
lineHeight: '3',
};
component.setProps({
rowHeightsOptions: { defaultHeight: { lineCount: 6 } },
rowHeightsOptions: rowHeightsOptionsWithLineHeight,
});
expect(setRowHeight).toHaveBeenCalled();
expect(setRowHeight).toHaveBeenCalledTimes(2);

component.setProps({
rowHeightsOptions: rowHeightsOptionsWithLineHeight,
gridStyles: { cellPadding: 'l' },
});
expect(setRowHeight).toHaveBeenCalledTimes(3);

component.setProps({
rowHeightsOptions: rowHeightsOptionsWithLineHeight,
gridStyles: { cellPadding: 'l', fontSize: 'l' },
});
expect(setRowHeight).toHaveBeenCalledTimes(4);
});

it('calculates undefined heights as single rows with a lineCount of 1', () => {
Expand All @@ -680,7 +741,7 @@ describe('EuiDataGridCell', () => {
callMethod(component);
expect(
mockRowHeightUtils.calculateHeightForLineCount
).toHaveBeenCalledWith(expect.any(HTMLElement), 1, false);
).toHaveBeenCalledWith(expect.any(HTMLElement), 1);
expect(setRowHeight).toHaveBeenCalled();
});

Expand Down
Loading
Loading