[EuiDataGrid] Add lineHeight to rowHeightsOptions

[andreadelrio]

The problem

EuiDataGrid calculates the height of multiline cells (when requested via rowHeightsOptions) using cellPadding, fontSize and a fake cell that gets temporarily added to the document. fontSize includes a value for line-height, as the associated CSS classes are defined using @include euiFontSize. This calculation works well for plain text. However, there are cases where the consumer might pass a different type of content that requires additional line-height for adequate readability. That is the case we encounter in Discover’s New Table where we use EuiDescriptionList (inline and compressed).

Incrementing the line-height of the content on the rendered cell doesn’t solve the issue because the cells have an inline style with the height that was previously calculated using the fake cell. In other words, if we set lineCount to 3 and then try to increase the line-height on EuiDescriptionList the content gets cut off and displays 2.x lines (because the height was calculated using a lower line-height value).

The proposed solution

This PR adds a new option to rowHeightsOptions called lineHeight. The default value for lineHeight is regular, with the option to set it to extra. An alternative would be to allow the consumer to pass any value as line-height if we think that is better. Either way, hopefully this PR sets the path forward for fixing this issue.

Notes

• Please let me know if there's a better way to define the line-height in Sass. Currently doing something like @include lineHeightFromBaseline(3.5);.

Related to

• [EuiDataGrid] rowHeightsOptions not breaking content when passing pixel value #5137

• https://github.com/elastic/kibana/pull/110225/files

Checklist

• Check against all themes for compatibility in both light and dark modes
• Checked in mobile
• Checked in Chrome, Safari, Edge, and Firefox
• Props have proper autodocs and playground toggles
• Added documentation
  - [ ] Checked Code Sandbox works for the any docs examples
  - [ ] Added or updated jest tests
  - [ ] Checked for breaking changes and labeled appropriately
  - [ ] Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[cchaos]

Please let me know if there's a better way to define the line-height in Sass. Currently doing something like @include lineHeightFromBaseline(3.5);.

That's perfect! 💯

My other comment just based on the PR description at the moment is about this line:

This PR adds a new option to gridStyle called lineHeight which will only affect multiline cells.

It would be great if we could add this style under the configuration for cell height instead of at the top level of the EuiDataGrid. That should head off any comments about it not working when using the default/common usage of single line cells.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5140/

[ryankeairns]

Thanks for putting this together @andreadelrio
The density of content is a common thread in the feedback around multi-line rows, thus far.

[andreadelrio]

This PR adds a new option to gridStyle called lineHeight which will only affect multiline cells.

It would be great if we could add this style under the configuration for cell height instead of at the top level of the EuiDataGrid. That should head off any comments about it not working when using the default/common usage of single line cells.

Ah! This makes total sense. Thanks for pointing that out @cchaos. I've moved lineHeight to rowHeightsOptions instead. There seem to be a couple of minor TS errors which should be easy to fix when the team reviews this.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5140/

[cchaos]

@andreadelrio Thanks for moving that configuration. I think this setting could really benefit from a "real world" example, like your use of EuiDescriptionList. I'd love it if we could have a second example on this docs page that showcases when/why you would need to increase the line-height. It would also make continuous testing easier to have a dedicated example.

[chandlerprall]

Thanks for describing the need, tracking down the cause, and putting together the proposal @andreadelrio !

I'm not sure we'd want to solve this through another rowHeightOptions value (and I also agree with Caroline that gridStyle isn't the right place either). Directionally, I see gridStyle controlling the overall/global look & feel of the grid, normalizing paddings/margins and other visual styles across the rows and cells. When finer control is needed, rowHeightOptions is great for setting a more specific height, globally or per-row, and those options of pixel values, line count, and upcoming 'auto'.

If an application wants to render 2 lines, the grid should adapt to changes in line height and update to reflect. In other words, as you've observed:

if we set lineCount to 3 and then try to increase the line-height on EuiDescriptionList the content gets cut off and displays 2.x lines

Seems more of a bug where the grid doesn't respond to that style change, and we should try to address the bug rather than push a work-around configuration. We'll have an engineer pick this up and explore, but it's good to keep this approach in mind if we can't get a more out-of-the-box approach to function as expected.

[ryankeairns]

To clarify, a solution here would be helpful in getting data grid set as the default for Discover. There are other blockers to that getting done, so I wouldn't go so far as to say this in itself is a "blocker", but the current styling is a consistent piece of feedback we're hearing.

I did some very crude hacking to get around it, as seen here: https://github.com/elastic/kibana/pull/110225/files , by setting the font size to 'm', then overriding Discover's data grid font size to XS via CSS.

[chandlerprall]

@constancecchen and I walked through the underlying issue (line heights in a cell's child DOM aren't respected), determined some requirements, and took a shot at a different approach for comparison.

@ryankeairns would the following work for discover's usage?

starting guidelines

• we want the grid to have a standard look & feel, but customizable by applications
• any customization by an app should be global (or close to it) across its instance, e.g. one row should have the same line height as another row
• simpler conceptions give better DX

what we did

you can see	at
branch	https://github.com/chandlerprall/eui/tree/feature/5140-grid-line-heights
commit	chandlerprall@57aa7b6
diff w/ master	https://github.com/elastic/eui/compare/master...chandlerprall:feature/5140-grid-line-heights?expand=1#diff

• added a new lineHeight parameter to rowHeightOptions outside the existing default / per-row definition, this lineHeight style is applied to the cells so it is used for determining a cell's lines-to-show calculation
• updated the row heights example to render every row with 2 lines with a 3rem line height

what we avoided

• didn't want to allow a different line height per row as that breaks the predictability/UX guideline; an app could still do weird things and provide a pixel value if it really really wants this functionality
• requiring calling setCellProps to provide a grid-global style value; felt this was poor DX, and it would have some performance implications
• adding flexibility to respond to any mix of content/dom nodes/line heights

questions

Should we move lineHeight to a larger abstraction? Since ultimately lineHeight is added on to the cell's wrapping div's styles, further widenings of the API would probably make sense as a cellProps object which would be applied globally to all cells, analogous and compared to applying the same values via setCellProps(). e.g. cellProps={{ style: { lineHeight: '1.2rem' } }}. This would also allow setting the line height via css class: cellProps={{ className: 'customLineHeight' }}

todo

To move forward with this approach, we'd need to better update the row height example & explain how the configurations interact.

[ryankeairns]

@chandlerprall yes, I think this covers our needs. I'm not certain what use case drove the nth-row support, but I do not see that being used for Discover. Andrea is back today, as well, and may have additional feedback.

Thank you!

[andreadelrio]

I like this proposed approach too. Feels better to tackle the issue from the root. Thanks for working on it so quickly.

@constancecchen and I walked through the underlying issue (line heights in a cell's child DOM aren't respected), determined some requirements, and took a shot at a different approach for comparison.

@ryankeairns would the following work for discover's usage?

starting guidelines

• we want the grid to have a standard look & feel, but customizable by applications
• any customization by an app should be global (or close to it) across its instance, e.g. one row should have the same line height as another row
• simpler conceptions give better DX

what we did

you can see at
branch https://github.com/chandlerprall/eui/tree/feature/5140-grid-line-heights
commit chandlerprall@57aa7b6
diff w/ master https://github.com/elastic/eui/compare/master...chandlerprall:feature/5140-grid-line-heights?expand=1#diff

• added a new lineHeight parameter to rowHeightOptions outside the existing default / per-row definition, this lineHeight style is applied to the cells so it is used for determining a cell's lines-to-show calculation
• updated the row heights example to render every row with 2 lines with a 3rem line height

what we avoided

• didn't want to allow a different line height per row as that breaks the predictability/UX guideline; an app could still do weird things and provide a pixel value if it really really wants this functionality
• requiring calling setCellProps to provide a grid-global style value; felt this was poor DX, and it would have some performance implications
• adding flexibility to respond to any mix of content/dom nodes/line heights

questions

Should we move lineHeight to a larger abstraction? Since ultimately lineHeight is added on to the cell's wrapping div's styles, further widenings of the API would probably make sense as a cellProps object which would be applied globally to all cells, analogous and compared to applying the same values via setCellProps(). e.g. cellProps={{ style: { lineHeight: '1.2rem' } }}. This would also allow setting the line height via css class: cellProps={{ className: 'customLineHeight' }}

todo

To move forward with this approach, we'd need to better update the row height example & explain how the configurations interact.

[cchaos]

💟 I'm just gonna set this one to draft since it seems like there might be a different/more scalable approach coming in that we'll wait for review on.

[ryankeairns]

@chandlerprall so that I can communicate the status with the Discover team, what are the next steps here? Will your branch become a new PR replacing this one?

[chandlerprall]

@chandlerprall so that I can communicate the status with the Discover team, what are the next steps here? Will your branch become a new PR replacing this one?

We'll push up a new branch & PR to replace. I'll go ahead and close. With the autoheight PR merging soon we'll be able to pick this up next.