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.

Sign up for GitHub

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][BugFix]: Fix visible overflowing truncated text #7793

Merged
merged 14 commits into from
Jun 5, 2024
Merged

[EuiDataGrid][BugFix]: Fix visible overflowing truncated text #7793

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
64b9fc8
fix: prevent truncated text from showing
mgadewoll May 28, 2024
9d2ffef
docs(storybook): add specific row height story
mgadewoll May 28, 2024
8bf1bbe
test(VRT): add reference images
mgadewoll May 28, 2024
4e4ca05
chore: add changelog
mgadewoll May 28, 2024
d38aa3e
refactor: revert style changes
mgadewoll May 29, 2024
7553f40
fix: add additional wrapper and split styles
mgadewoll May 29, 2024
25c4c71
docs(storybook): add additional stories for row height specific settings
mgadewoll May 29, 2024
48c934f
test(VRT): add reference images for new stories
mgadewoll May 29, 2024
c47968c
test: update snapshots
mgadewoll May 29, 2024
3f67373
refactor(EuiDataGrid): add Cees suggestion to use transparent border
mgadewoll Jun 4, 2024
742961a
test: revert snapshots
mgadewoll Jun 4, 2024
de2cfd5
fix: ensure gridStyle fontSize and cellPadding trigger recalculation …
mgadewoll Jun 4, 2024
9f0996d
chore: revert gridStyles update fix
mgadewoll Jun 4, 2024
bce20ea
changelog copy tweak
cee-chen Jun 4, 2024
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
Binary file added ...oki/reference/chrome_desktop_Tabular_Content_EuiDataGrid_Custom_Row_Heights.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...loki/reference/chrome_desktop_Tabular_Content_EuiDataGrid_Height_Line_Count.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...s/eui/.loki/reference/chrome_desktop_Tabular_Content_EuiDataGrid_Row_Height.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...loki/reference/chrome_mobile_Tabular_Content_EuiDataGrid_Custom_Row_Heights.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ....loki/reference/chrome_mobile_Tabular_Content_EuiDataGrid_Height_Line_Count.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...es/eui/.loki/reference/chrome_mobile_Tabular_Content_EuiDataGrid_Row_Height.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions packages/eui/changelogs/upcoming/7793.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
**Bug fixes**

- Fixed a visual bug on `EuiDataGrid` cells when using `rowHeightOptions.defaultHeight.lineCount` where the clamped text was still visible for some font sizes
cee-chen marked this conversation as resolved.
Show resolved Hide resolved

228 changes: 174 additions & 54 deletions packages/eui/src/components/datagrid/__snapshots__/data_grid.test.tsx.snap
View file
Open in desktop

Large diffs are not rendered by default.

32 changes: 24 additions & 8 deletions packages/eui/src/components/datagrid/body/__snapshots__/data_grid_body_custom.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,14 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
hello
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
hello
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand All @@ -158,10 +162,14 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
world
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
world
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand All @@ -188,10 +196,14 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
lorem
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
lorem
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand All @@ -214,10 +226,14 @@ exports[`EuiDataGridBodyCustomRender treats \`renderCustomGridBody\` as a render
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
ipsum
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
ipsum
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand Down
24 changes: 16 additions & 8 deletions ...s/eui/src/components/datagrid/body/__snapshots__/data_grid_body_virtualized.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,16 @@ exports[`EuiDataGridBodyVirtualized renders 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
<span>
cell content
</span>
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
<span>
cell content
</span>
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand All @@ -161,12 +165,16 @@ exports[`EuiDataGridBodyVirtualized renders 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
<span>
cell content
</span>
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
<span>
cell content
</span>
</div>
</div>
<p
class="emotion-euiScreenReaderOnly"
Expand Down
28 changes: 16 additions & 12 deletions packages/eui/src/components/datagrid/body/cell/__snapshots__/data_grid_cell.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,24 @@ exports[`EuiDataGridCell renders 1`] = `
tabindex="-1"
>
<div
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight eui-textTruncate"
class="euiDataGridRowCell__content euiDataGridRowCell__content--defaultHeight"
data-datagrid-cellcontent="true"
>
<div>
<button
data-datagrid-interactable="true"
>
hello
</button>
<button
data-datagrid-interactable="true"
>
world
</button>
<div
class="euiDataGridRowCell__contentInner eui-textTruncate"
>
<div>
<button
data-datagrid-interactable="true"
>
hello
</button>
<button
data-datagrid-interactable="true"
>
world
</button>
</div>
</div>
</div>
<p
Expand Down
52 changes: 31 additions & 21 deletions packages/eui/src/components/datagrid/body/cell/data_grid_cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,15 @@ const EuiDataGridCellContent: FunctionComponent<
() =>
classNames(
'euiDataGridRowCell__content',
`euiDataGridRowCell__content--${cellHeightType}Height`,
`euiDataGridRowCell__content--${cellHeightType}Height`
),
[cellHeightType]
);

const innerContentClasses = useMemo(
() =>
classNames(
'euiDataGridRowCell__contentInner',
!isControlColumn && {
'eui-textBreakWord': cellHeightType !== 'default',
'eui-textTruncate': cellHeightType === 'default',
Expand All @@ -101,28 +109,30 @@ const EuiDataGridCellContent: FunctionComponent<

return (
<>
<RenderTruncatedCellContent
hasLineCountTruncation={
cellHeightType === 'lineCount' && !isControlColumn
}
rowHeight={rowHeight}
<div
ref={setCellContentsRef}
mgadewoll marked this conversation as resolved.
Show resolved Hide resolved
data-datagrid-cellcontent
className={classes}
>
<div
ref={setCellContentsRef}
data-datagrid-cellcontent
className={classes}
<RenderTruncatedCellContent
hasLineCountTruncation={
cellHeightType === 'lineCount' && !isControlColumn
}
rowHeight={rowHeight}
>
<CellElement
isDetails={false}
data-test-subj="cell-content"
rowIndex={rowIndex}
colIndex={colIndex}
schema={column?.schema || rest.columnType}
{...cellContext}
{...rest}
/>
</div>
</RenderTruncatedCellContent>
<div className={innerContentClasses}>
Copy link
Contributor Author

@mgadewoll mgadewoll May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I generally don't like adding additional wrappers but we need to separate the line-clamp from the padding to ensure that the truncated text is hidden.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my hangup is... I just honestly don't feel like it's worth it 🙈 it feels like an outsized performance impact (extra DOM node per cell, which will scale enormously on datagrids) for what's purely a visual effect. I'm tempted to look into other approaches if possible (pseudo element that "covers" the text? some other overflow CSS workaround?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I found the world's hackiest CSS workaround using a transparent border! Let me know what you think? main...cee-chen:eui:datagrid/line-clamp-css-workaround

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I totally agree - adding a wrapper is a big performance impact on DataGrid. That's why I'm also not really happy with it.
I checked your hacky workaround and it seems to work great! 🪄 I'd vote for that one as it's definitely way more lightweight 🪶 🎉

Copy link
Contributor Author

@mgadewoll mgadewoll May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One small downside I notice is that it does not currently update when changing the controls in the story (e.g. gridStyle.fontSize - it renders correctly on reload though.)

Copy link
Contributor

@cee-chen cee-chen May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After poking at it, I think this is an issue in production datagrid because gridStyles.fontSize doesn't correctly update the expected row height on the production storybook either (which is the underlying issue).

I'm not totally sure this is worth chasing down right now as consumers typically don't change custom gridStyle.fontSize on the fly (vs users using the density toolbar control). Am I way off on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I reverted the wrapper changes in favor of your suggested transparent border hack here in this commit.

  2. I agree that the gridStyle issue is not a storybook issue but the prop generally doesn't trigger a recalculation.

In general this should not be an issue, as production would likely never on the fly change the styles via gridStyle prop, but in Storybook it seems weird/broken as we showcase the prop object and IMHO it's not apparent it's expected to not update. 😐

I gave it a quick shot to ensure the cell height calculation is triggered on fontSize and cellPadding change here in this commit. WDYT? (We don't have to implement this here if you don't agree with it, it's a suggestion in case we can fix it this way rather easily 🙂)

Copy link
Contributor

@cee-chen cee-chen Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

😬 I would kinda strongly prefer not to in this PR - if we do want to resolve it, I'd prefer it be a separate issue + PR to make it easy to roll back standalone if necessary.

For context, I'm generally paranoid/conservative about anything involving EuiDataGrid due to its scale which results in outsized rerender/performance issues, and the cascading impact the component has on power users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's fair. I'll revert the last commit and move it to a separate PR then 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted in this commit.

ℹ️ I added a separate issue for the gridStyles update.

<CellElement
isDetails={false}
data-test-subj="cell-content"
rowIndex={rowIndex}
colIndex={colIndex}
schema={column?.schema || rest.columnType}
{...cellContext}
{...rest}
/>
</div>
</RenderTruncatedCellContent>
</div>

<EuiScreenReaderOnly>
<p hidden={!isFocused}>
Expand Down
157 changes: 156 additions & 1 deletion packages/eui/src/components/datagrid/data_grid.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ const meta: Meta<EuiDataGridProps> = {
minSizeForControls: MINIMUM_WIDTH_FOR_GRID_CONTROLS,
},
};
enableFunctionToggleControls(meta, ['onColumnResize']);

export default meta;
type Story = StoryObj<EuiDataGridProps>;
Expand Down Expand Up @@ -299,6 +298,162 @@ export const Playground: Story = {
},
render: (args: EuiDataGridProps) => <StatefulDataGrid {...args} />,
};
enableFunctionToggleControls<EuiDataGridProps>(Playground, ['onColumnResize']);

export const HeightLineCount: Story = {
parameters: {
controls: {
include: ['gridStyle', 'rowHeightsOptions', 'width', 'height'],
},
},
args: {
columns,
rowCount: 5,
renderCellValue: RenderCellValue,
// setup for easier testing/QA
columnVisibility: {
visibleColumns: [
'name',
'email',
'account',
'location',
'date',
'amount',
'phone',
'version',
],
setVisibleColumns: () => {},
},
gridStyle: {
fontSize: 's',
cellPadding: 'm',
border: 'all',
stripes: false,
header: 'shade',
footer: 'overline',
stickyFooter: true,
rowHover: 'highlight',
rowClasses: {},
},
width: '700px',
height: '',
toolbarVisibility: false,
rowHeightsOptions: {
defaultHeight: {
lineCount: 1,
},
lineHeight: undefined,
scrollAnchorRow: undefined,
},
},
render: (args: EuiDataGridProps) => <StatefulDataGrid {...args} />,
};

export const RowHeight: Story = {
parameters: {
controls: {
include: ['gridStyle', 'rowHeightsOptions', 'width', 'height'],
},
},
args: {
columns,
rowCount: 5,
renderCellValue: RenderCellValue,
// setup for easier testing/QA
columnVisibility: {
visibleColumns: [
'name',
'email',
'account',
'location',
'date',
'amount',
'phone',
'version',
],
setVisibleColumns: () => {},
},
gridStyle: {
fontSize: 's',
cellPadding: 'm',
border: 'all',
stripes: false,
header: 'shade',
footer: 'overline',
stickyFooter: true,
rowHover: 'highlight',
rowClasses: {},
},
width: '',
height: '',
toolbarVisibility: false,
rowHeightsOptions: {
defaultHeight: {
height: 48,
},
rowHeights: {},
lineHeight: undefined,
scrollAnchorRow: undefined,
},
},
render: (args: EuiDataGridProps) => <StatefulDataGrid {...args} />,
};

export const CustomRowHeights: Story = {
parameters: {
controls: {
include: ['gridStyle', 'rowHeightsOptions', 'width', 'height'],
},
},
args: {
columns,
rowCount: 5,
renderCellValue: RenderCellValue,
// setup for easier testing/QA
columnVisibility: {
visibleColumns: [
'name',
'email',
'account',
'location',
'date',
'amount',
'phone',
'version',
],
setVisibleColumns: () => {},
},
gridStyle: {
fontSize: 's',
cellPadding: 'm',
border: 'all',
stripes: false,
header: 'shade',
footer: 'overline',
stickyFooter: true,
rowHover: 'highlight',
rowClasses: {},
},
width: '700px',
height: '',
toolbarVisibility: false,
rowHeightsOptions: {
defaultHeight: {
lineCount: 1,
},
rowHeights: {
2: 'auto',
3: 48,
4: {
height: 48,
},
},
lineHeight: undefined,
scrollAnchorRow: undefined,
},
},
render: (args: EuiDataGridProps) => <StatefulDataGrid {...args} />,
};

const StatefulDataGrid = (props: EuiDataGridProps) => {
const { pagination, sorting, columnVisibility, ...rest } = props;
Expand Down
Loading