[DataGrid] Enable Auto Fit behavior when Cell row height - Custom is selected

[kertal]

Is your feature request related to a problem? Please describe.

When configuring a Custom - Cell row height in EuiDataGrid, depending on the displayed content, this can lead to lots of whitespace, e.g. when there's a lot of short content to be displayed.

Example:

When configuring an Auto fit - Cell row height in EuiDataGrid, this can lead to very large rows, when there's lots of text that needs to be displayed. E.g. JAVA Error messages.

For very short and very long content no current mode is optimal.

Describe the solution you'd like

For the optimal usage of space, it would be great if the Custom - Cell row height would treat the setting as max cell height to be displayed. But if all the cells have just a single line of content, it would be treated like Auto fit. This would provide improve the displayed information density. Since this would significantly change a given behavior of the grid, it could be a configuration flag, enabling this behavior

Describe alternatives you've considered
Provide a separate setting for the Auto fit mode, so it configured what's the maximum height a cell can have, to prevent Skyscraper like cells.

Desired timeline
Since we are working on improving the Logs experience, in Discover working on Contextual Awareness and for this, better information density is essential, sooner is better than later.

[JasonStoltz]

@kertal Are you able to provide a codesandbox that illustrates this behavior? I'm having trouble understanding.

[kertal]

@JasonStoltz best to show you in our 1:1 tomorrow :)

[JasonStoltz]

I just spoke with @kertal and he mentioned that there are potentially 2 solutions for this:

1. Add a "max" option to "autofit". Meaning, meaning "autofit" could be adjusted to accept a maximum number of lines displayed.

2. We could change the way that "custom" works, so that instead of being a fixed value for each row, it is actually a max for each row.

@cee-chen mentioned that there is an implicit trade-off between "autofit" and "custom" currently:

• "autofit" is much less performant because of the additional calculations that must be made for every row in the datagrid
• "custom" is much more performant because there are no calcuations required

Considering the above, I would propose that option 1 is the preferable option as it is just a simple tweak to the existing calculation for autofit. I do not believe this would have a considerable impact on "autofit"'s performance and "custom" would remain the simple, performant option.

@cee-chen Does that sound doable? And would you size this as a small, medium, or large effort?

@MichaelMarcialis Does that sound reasonable from a UX perspective?

[tkajtoch]

If there are any performance considerations regarding this change, we could use our componentDefaults feature to conditionally enable this based on a feature flag in Kibana.

[JasonStoltz]

@kertal Hey Matthias. I discussed this a bit with @cee-chen. We came up with a few additional points and questions --

1. Why does this need to be a user setting? It it possible to do limit the size of data values programmatically before rendering them in the data grid? Or apply line clamp css yourselves?
2. Do we think this is actually a feature that users will find and use? It's still feeling like an edge case.
3. Just FYI, we do think this could have a performance impact with either of the two proposed options. If we do option 1, we'd have to do line-height calculations in addition to the height calculations we currently do. If we go with option 2, we need to do height calculations in addition to line-height. I don't know if this is a blocker or not, but just wanted to mention that it is a little bit tricker than I originally made it sound.

I set up some time for us to talk through this in a few weeks. But if we can figure some of this out async in this issue ahead of then that works too!

[MichaelMarcialis]

Does that sound reasonable from a UX perspective?

If I may, I'd like to make a pitch for the 2nd solution you mention above, using a UI along the lines of this screenshot:

My thinking is as follows:

• The "Single" row height option never made much sense to me, as it is technically the same as a "Custom" row height set to 1 line. So for the sake of simplicity, why don't we remove the "Single" option altogether?

• With the space we gain from removing the "Single" option, we can absorb the line number input (which currently appears on it's own separate form row) into the preceding form row, immediately after the "Row height" button group. When "Custom" is selected, the line number input is enabled. When "Autofit" is selected, the line number input is disabled.

• Unless I'm mistaken, I feel like the current behavior of showing all rows at the same line height when "Custom" is selected (regardless if the cell content within exceeds that number) is not desirable behavior for our users. For example, in @kertal's screenshot above, I can't think of many scenarios where users would be find that acceptable. The only scenario that comes to mind would be to prove additional whitespace between rows (which is not something I've heard requested by users and something that the density option should likely be used for instead). As such, I think the more desired default behavior would be to change all custom line heights to be treated as a maximum value. If the height of the cell contents are less than the populated custom height, the row should shrink to hug the content.

• I'm not a fan of the 1st option (i.e. add a "max" option to "autofit"), as it feels like two settings that are at odds with each other. If I want my row height to autofit my content, then the user likely doesn't wish to impose a maximum line count. On the other hand, if they do wish to impose a maximum line count, that's not technically what I would consider "autofitting" the rows for their content. It's a bit confusing, IMO.

[ninoslavmiskovic]

@JasonStoltz I can answer on the user usage. We are expecting users will use it because we need it as part of making Discover context aware and where users will be exploring several different data types that requires the Max to make the data density optimal.

Do I understand the proposal currently that if we go with upgrading auto fit with max then it would be a small task?

[JasonStoltz]

@ninoslavmiskovic

Do I understand the proposal currently that if we go with upgrading auto fit with max then it would be a small task?

Regardless of what path we choose, I don't believe it would be a small task, but also not huge. I would estimate this at 1-2 weeks of work. Besides the actual effort, it will require due diligence to ensure don't impact rendering performance.

because we need it as part of making Discover context aware and where users will be exploring several different data types that requires the Max to make the data density optimal.

Just to rephrase that to check my own understanding: We won't know what sort of data is being used by Discover, so we need to offer more row height customizations so that users can find a view that works best for them. We on the Kibana side can't assume the type of data being viewed.

@MichaelMarcialis It's a good point Michael. Changing the default of Custom may be the right move here, unless there is a clear use case where users would want all lines uniformly the same height. Changing the default seems to be less clicks and more intuitive for users.

I'm neutral on whether or not to keep "Single". It sounds like if we remove it, the default become "Custom: 1" as seen in your screenshot. Even though "Single" is functionally equivalent to "Custom: 1", it does seem a bit more intuitive to just click "Single". I'm good any way though.

@tkajtoch If you get a chance, can you poke around at this technically and see if you're able to give us any technical insight on the level of effort here?

[MichaelMarcialis]

@ninoslavmiskovic: Given that @JasonStoltz's estimated LOE of the two options is approximately the same, determining the best user experience appears to be the only real deciding factor. Unless we have a known use case to continue having "Custom" row heights behave as absolute and uniform, then my personal recommendation is to proceed with what I've recommended above. Do you agree? Or do you have any concerns with that approach? Just let us know!

Also CCing @kertal, @andreadelrio, @ryankeairns, in case they wish to weigh in as well.

[ninoslavmiskovic]

@MichaelMarcialis I like it - fewer click for the users. Do you have any concerns @kertal or @davismcphee ?

[davismcphee]

++ to @MichaelMarcialis' suggestion. I don't see much if any value to users in having absolute custom row heights, and I like the simplification of the settings UI.

[kertal]

++ from my end for @MichaelMarcialis suggestion, it's a nice simplification and definitely makes sense for Discover.

If there are performance concerns, or concern that it might be a breaking change, it could be introduced with a feature flag like @tkajtoch suggested:

If there are any performance considerations regarding this change, we could use our componentDefaults feature to conditionally enable this based on a feature flag in Kibana.

We could measure the performance impact with a performance profile. So far we haven't heard complains since introducing Auto-Fit in Discover, but it's also not the default setting.

[kertal]

One ask @MichaelMarcialis, according to your mocks, you ask for the same for the header cell lines, right?

[MichaelMarcialis]

One ask @MichaelMarcialis, according to your mocks, you ask for the same for the header cell lines, right?

Good question. The mockup I shared in my comment above was intended to show my thoughts for the default EUI data grid options. For Discover's specific use case, I think we can continue to have separate controls for header and body (per the mockups for Discover content-aware logs).

[JasonStoltz]

I met with @MichaelMarcialis @kertal and @cee-chen to talk through this one a bit.

Our primary concern is still around performance.

The EUI team agreed to make this change initially as a "spike" which we can hand off to @kertal and team to do some testing to make sure there won't be significant performance impact as a result of this.

If we can validate that the performance won't be a blocker, we can proceed with this change.

[kertal]

@cee-chen I've a question around performance.

What should we aim to configure for testing. For context, I've been comparing performance when there are 2 columns configured, pagination is disabled, 10.000 rows are returned via ES|QL

it was pretty fast, didn't test multiple times, so number may vary. But I guess it's the case when there are multiple columns configured?
Left side (Auto Fit, should be more expensive)
Right side (Custom, should not need to measure and is therefore cheaper)

in this case Auto Fit was a bit faster, however it's just a quick test, not isolated, not done multiple times, but sufficient to start a conversion

Then I tested with 10 columns, again, it was fast (Auto fit left, Custom, right), a bit more calculaton time on the auto fit side, but very little

To test it:
https://kertal-pr-187964-remove-pagination.kbndev.co/app/r/s/sZv3J

FYI @flash1293 @davismcphee @stratoula , so if we want to remove pagination in Discover/ES|QL , I don't think in terms of performance it's a problem. Loading 10000 records in my setup and with the given test data doesn't seem to be a problem

[flash1293]

Thanks for confirming @kertal ! I still think it's the right call, let's discuss with @ninoslavmiskovic once he's back

[JasonStoltz]

We should handle #7951 when we work on this one.

[cee-chen]

@kertal @MichaelMarcialis Apologies that it's been a while since we picked this issue up - but just to clarify, is the scope of this PR just to remove the Single button from the display selector and inline the number input?

As in: we're not enabling auto-fit height for the lineCount row height style, correct?

[kertal]

In my understanding the original intend of this issue would be to enable auto-fit height for the line count, and coming with that to remove the single button ... it might make sense to split this up

[cee-chen]

@kertal Do you mind providing a timeline/priority for the two separate requests (first is simplifying the control/selector UI, and the second is actually changing how the lineCount row height behaves)? We can certainly do the first by end of Q2, but I still have a lot of reservations around the second and shipping with it as OOTB behavior.

[kertal]

Priority wise changing the lineCount behavior I'd consider as having a bigger impact. But we wanted to test / feature flag it, test it in Discover before enabling as OOTB behavior, didn't we? 🙏 (FYI @timductive )

[cee-chen]

This functionality is now achievable via a feature flag in the rowHeightsOptions prop:

<EuiDataGrid
  rowHeightsOptions={{
    autoBelowLineCount: true,
    defaultHeight: { lineCount: 3 },
  }}
  // ... rest of props
/>

Also see https://eui.elastic.co/pr_8096/storybook/?path=/story/tabular-content-euidatagrid-rowheightsoptions-prop--auto-below-line-count for an example of the behavior in the UI.

[davismcphee]

Thanks @cee-chen! This is awesome news and much appreciated 🙏

[kertal]

Thx @cee-chen 🙇 that's great!