-
Notifications
You must be signed in to change notification settings - Fork 840
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] Add lineHeight to rowHeightsOptions #5140
Conversation
That's perfect! 💯 My other comment just based on the PR description at the moment is about this line:
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. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5140/ |
Thanks for putting this together @andreadelrio |
Ah! This makes total sense. Thanks for pointing that out @cchaos. I've moved |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5140/ |
@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. |
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 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:
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. |
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. |
@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
what we did
what we avoided
questionsShould we move todoTo move forward with this approach, we'd need to better update the row height example & explain how the configurations interact. |
@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! |
I like this proposed approach too. Feels better to tackle the issue from the root. Thanks for working on it so quickly.
|
💟 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. |
@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. |
The problem
EuiDataGrid calculates the height of multiline cells (when requested via
rowHeightsOptions
) usingcellPadding
,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
andcompressed
).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 onEuiDescriptionList
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
calledlineHeight
. The default value forlineHeight
isregular
, with the option to set it toextra
. 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
@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
- [ ] 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