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

[Lens] Datatable - make 3 lines the default setting for header row height #172698

Closed
mbondyra opened this issue Dec 6, 2023 · 5 comments · Fixed by #180505
Closed

[Lens] Datatable - make 3 lines the default setting for header row height #172698

mbondyra opened this issue Dec 6, 2023 · 5 comments · Fixed by #180505
Assignees
Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@mbondyra
Copy link
Contributor

mbondyra commented Dec 6, 2023

To align Discover behavior with Lens, we want to change the Header row height setting from single to 3 lines.
Here's related issues for Discover:

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Dec 6, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@stratoula stratoula added discuss enhancement New value added to drive a business result impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed discuss labels Dec 6, 2023
@mbondyra mbondyra self-assigned this Apr 10, 2024
@markov00
Copy link
Member

markov00 commented Apr 10, 2024

I'm not fully sure I've understood where this default will be changed.
In Lens table you have 3 ways to adjust the header row height: single,auto fit and custom.
Not the default is single. Are you going to change it to Custom:3 or in the single format you are going to change it to 3?

There is also another thing to notice: the custom property behaves differently from the cell height. In the header, the custom value is considered the max possible number of rows, wherein the cell height, value is considered as "the" value to use, independently from the context

@mbondyra
Copy link
Contributor Author

mbondyra commented Apr 10, 2024

Hi Marco, if user didn't change a setting in any way, it will by default display custom, 3. This is how it works also in Discover now.
Screenshot 2024-04-10 at 18 39 25

There is also another thing to notice: the custom property behaves differently from the cell height. In the header, the custom value is considered the max possible number of rows, wherein the cell height, value is considered as "the" value to use, independently from the context

I am not sure why it is like that, it might be a EuiDataGrid limitation? Or maybe it's just prettier this way because the rows are not different height. The behavior is the same on Discover though. Please feel free to open an issue about it or let's sync with eui and discover before deciding if we need to change anything here. @davismcphee maybe you know?

@davismcphee
Copy link
Contributor

In EuiDataGrid, rowHeightOptions accepts a lineHeight value that determines how tall a single line is expected to be. If the consumer specifies a lineCount value (such as 3), it will render all cell row heights in the grid as lineHeight * lineCount, presumably for consistency when scrolling through rows (it might also have to do with virtualization).

On the other hand, we use the -webkit-line-clamp CSS property when specifying a header row height, which as mentioned only sets the max number of lines.

So the behaviour is technically inconsistent, but I'm not sure adding extra height to the header rows for consistency would be better since headers rows are always fixed to the top of the grid, so it would really just take extra space from the grid cells even though it's not actually being used.

@markov00
Copy link
Member

Agree, I don't think we need to align these behaviours currently. I was just noticing this difference.
Also I don't technically see why we have the single option since we have also the ability to set the custom:1, but anyway all good for now :D

mbondyra added a commit that referenced this issue Apr 16, 2024
…ight (#180505)

## Summary

Fixes #172698

The default setting for the header row height is custom, 3 lines (so if
user doesn't modify the header row height). It doesn't visually affect
datatables with short labels.

<img width="765" alt="Screenshot 2024-04-10 at 18 39 51"
src="https://github.com/elastic/kibana/assets/4283304/bdec216b-36e4-4ca4-8135-85008b7f311d">

<img width="739" alt="Screenshot 2024-04-10 at 18 39 25"
src="https://github.com/elastic/kibana/assets/4283304/e5e6c4d5-f9c6-4270-85ba-f0ada2fbea69">

To code reviewers - this PR in itself is quite small but I rewrote all
the datatable tests to testing library and made some code style
improvements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants