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.
Original file line number Diff line number Diff line change
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
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 @@ -96,14 +96,7 @@ const EuiDataGridCellContent: FunctionComponent<
const cssStyles = [
styles.content.euiDataGridRowCell__content,
...(isControlColumn
? [
// Control column cells should not be vertically centered (defaultHeight) except
// on single rows. They should be top-aligned for auto and lineCount heights
styles.content.controlColumn,
cellHeightType === 'default'
? styles.content.defaultHeight
: styles.content.autoHeight,
]
? [styles.content.controlColumn, styles.content.autoHeight]
: [
// Regular data cells should always inherit height from the row wrapper,
// except for auto height
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,16 @@ export const CustomLineHeight: Story = {
<StatefulDataGrid {...storyArgs} rowHeightsOptions={rowHeightsOptions} />
),
};

// Visual regression test for https://github.com/elastic/eui/issues/7897
// Control column checkboxes & buttons should vertically align with the first
// line of text for both single and multiple lines of text
export const CustomLineHeightControlColumn: Story = {
tags: ['vrt-only'],
render: () => (
<StatefulDataGrid
{...storyArgs}
rowHeightsOptions={{ lineHeight: '3', defaultHeight: { lineCount: 2 } }}
/>
),
};