elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.47k stars 8.04k forks source link

[OneDiscover] Add density configuration to unified data table #186007

Open kertal opened 3 weeks ago

kertal commented 3 weeks ago

Describe the feature:

To make the information density configureable add the EuiDataGrid provided showDisplaySelector.allowDensity configuration to Discover's UnifiedDataTable.

Bildschirmfoto 2024-06-11 um 18 41 39

Notes:

elasticmachine commented 3 weeks ago

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

davismcphee commented 3 weeks ago

@lukasolson As we discussed in our 1-1, I've assigned this to you to work on.

Looking into this a bit, it looks like the density options are hardcoded in the EuiDataGrid density selector, and none of the options match the current Discover default fontSize: 's', cellPadding: 'l'. This creates a challenge since it means no density option would be selected by default, and once the user changes it, they'd have no way to get back to the default.

These are the hardcoded EUI options:

There are a couple of ways we could approach this:

Curious to hear thoughts on this.

cc @kertal @ninoslavmiskovic @andreadelrio

davismcphee commented 3 weeks ago

Another product question for @ninoslavmiskovic: we currently persist the user's data grid configurations when they save a search. I'm assuming we'd want to do the same here for consistency, but I want to confirm that with you first.

ninoslavmiskovic commented 3 weeks ago

Yes 👍 all configuration should persist .

ninoslavmiskovic commented 3 weeks ago

Tagging @MichaelMarcialis since he did the design on this one ☝️

kertal commented 3 weeks ago

I have a preference what we shouldn't do

Implement our own density selector like we've done for the row height selectors for similar reasons (this would also allow us to rearrange the order in the popover, which isn't possible with the default EUI options currently) - medium effort

IMO we should use the EUI options, or extend them

ninoslavmiskovic commented 3 weeks ago

I agree with @kertal -👍

davismcphee commented 2 weeks ago

Ok sounds good. So in that case we will need to decide if we are ok with using the hardcoded EUI options, which will change the default density of the Discover data grid, or if we should make an EUI contribution to support configurable density options in the data grid. Probably a question best answered by our design friends @andreadelrio and @MichaelMarcialis.

MichaelMarcialis commented 2 weeks ago

Good discussion. The design mockups I created for the One Discover context aware UX project are nearly identical to the EUI provided "Compact" density. The only difference is that I have some additional horizontal padding applied to the cells (8px instead of 4px). I had suggested this additional horizontal padding because I felt that it aided with overall legibility at that size.

That said, if we are unable to customize those density padding values, I suppose we could try to utilize the default EUI densities to see if they would be viable. We can then determine whether we need to contribute a change to EUI or not. Here's a quick example of the two next to each other, to inform opinions:

CleanShot 2024-06-17 at 15 05 09

To be honest, they are very close. I think I'd be OK with just proceeding with the EUI provided densities. Thoughts?

davismcphee commented 2 weeks ago

@MichaelMarcialis On my end I'm totally fine with using the existing EUI density options in Discover, and I agree the two compact density designs are very similar anyway.

I think the main thing to consider is that currently Discover is using a custom combo of density options which doesn't closely map to any of the available EUI options: fontSize: 's', cellPadding: 'l'. If we were to go with the available EUI options, we'd need to change the default Discover density to one of the available options instead (which I'm also fine with, just want to make sure we're aware and that we decide which to use by default).

kertal commented 2 weeks ago

+1 from my end going with EUI defaults, and if necessary applying the change in EUI

ninoslavmiskovic commented 2 weeks ago

+1 on going with EUI