[EuiDataGrid] Add row height switcher to column

[timroes]

We discussed this briefly in our Discover sync yesterday and @snide suggested that it might make sense moving the row height toggle into EUI itself, since we might need this (in an aligned way) across different places in Kibana.

So we'd like to ask adding a row height switcher to the data grid, that looks similar to the following:

I think we need the following properties exposed on EUI to configure it from the outside:

• initialRowHeight: 'auto' | 1 | 3 | 5 - to pass in which row height should be selected by default. I'd suggest we keep 1 as default if it's not passed in.
• onRowHeightChanged(newVal: 'auto' | 1 | 3 | 5) - to get notified if the user changes the row height, since we'll be storing this in Discover with the saved search
• (optional): maybe there are use-cases why the user would like to configure the available options, and would like to change the row heights options available, in which case we'd need to expose that too. Discover currently wouldn't have that requirement if we go with auto | 1 | 3 | 5.

cc @ryankeairns

[snide]

For the design of this I would update the treatment as follows:

• Choices should be auto | number using a number input for the row height, rather than a selector. Agree with a default setting of 1
• Button name should be auto row height | row height of 3 and adjust based upon the selection (which is in line with how the other buttons behave)
• Full-height should remain the last item in the toolbar

[ryankeairns]

FWIW, Discover will need to add custom content to that popover.

Specifically, a switch that applies the row height option to only their default view. That's product specific, so I think what I'm asking for is a means to add children form elements/content below the row height settings, like so:

Note: don't take this screenshot as UI design guidance. I shared it only to demonstrate the need for additional content below the row height inputs Dave described above.

[ryankeairns]

@chandlerprall for the sake of clarity, and after the lineHeght addition (ty!), this is the last piece of UI/component change needed toward completion of the data grid rollout checklist for Discover.

The remaining items are final review QA checks. Thanks!

[cee-chen]

Hey @miukimiu! Do you mind taking a design pass on this from Dave and Ryan's comments? I'd like to start working on this but I'm a little confused on what it should look like (Dave's comment seems to suggest a dropdown or number input of some sort, whereas the existing screenshots suggest a button group). I'm also uncertain on the custom content/children requirement and would love to brainstorm that.

[ryankeairns]

I'm happy to provide any additional context, as needed.

[miukimiu]

Hi @constancecchen ,

This is what it should look like if we all agree (Figma file):

I'm also uncertain on the custom content/children requirement and would love to brainstorm that.

@ryankeairns can you provide additional context? What is the "default view only"? From a design perspective the additional content could be restricted to a EuiFormRow display="columnCompressed".

[snide]

Default view in this case would be the display of the table when no columns are selected in discover (it's essentially a blob). In that format, you generally want the row height to be larger, and in the column view you want them to be smaller. I think possible we should just auto set that for users with a good default rather than giving them a toggle for it (which I think might be overly complex).

Here was an alternative I had that might be a little easier to grok at first glance. It omits the "default view" stuff. Whichever way we go, I'd suggest using the words "Lines per row" rather than "number of rows" because it's a little more clear. Feel free to ignore or slant it @miukimiu or @ryankeairns. Just had some downtime.

[ryankeairns]

Apologies for the late reply!

Having the default view 'auto-fit' makes sense although (not looking up how exactly that works) we'd want to insure it is capped since the Document field can be very long (does that initial blob cut off at 5 lines?). The 'max' issues is likely an implementation Kibana-side concern. There is some feedback supporting auto-fit / multi-lines per row as the default. Also, this is how it has long worked in Discover :)

Anyway, back to the switch. This switch is rather Discover-specific, in my mind. You start with auto-fit / 5 lines in the default view and, upon adding a column, the data grid would change to 1 line per row. This switch determines whether or not the switch to a 1 line per row happens or if you retain the auto-fit or X lines per row setting. The preference here is less conclusive, thus the option.

Maybe we set the switch question aside for the sake of this PR and just focus on the auto-fit / set number of lines per row UI. This is what I was implying by suggesting the possibility of adding children elements (i.e. let Discover decide what to do about the switch).

[cee-chen]

Thanks y'all, I'm ++ to leaving off the custom content for now until we figure out exactly what we want from it.

Also ++ to @snide's copy suggestion on "Lines per row" rather than "number of rows", agreed it's less confusing for me at least. Any thoughts or pushes as to which direction (@miukimiu or @snide)'s implementation I should go with? I will say that in Dave's first element, it wasn't clear to me that the number "3" was separate from the "autofit to content" action or that it would cause the number input to go away completely - I think I'd prefer more visual separation personally.

[ryankeairns]

@shaunmcgough and @timroes any thoughts on the switch? To this point, it seemed like something we wanted to provide as an option (to switch to a single line in the custom table state / after adding a column(s)). I think what that means here is the component (specifically the row height switcher UI) allows you to pass in child elements to the popover menu.

@constancecchen +1 for a more distinct separation. Here is what I'm envisioning:

[timroes]

@ryankeairns For me the component looks nice that way. I am just curious if we'll actually need that separate switch for "apply to default view only", since I am not 100% sure if users will really get what "default view" means. Or if we should just switch automatically to 1 line when a user switches to have configured columns and in case they remove the last one, back to our default for "default view", i.e. always have the user to configure it in whatever view they are in, without the ability to configure that it only applies for "default view". Also if the user would not be in default view anymore (i.e. has columns configured), would she still see that switch or would it now be "Apply to configured/current view only". In general I feel like this might cause more confusion for users than the benefits of it, and wonder if we should just try going without it for the beginning, and figure out if we're wanting to do that later if we're getting feedback from users, that it's annoying they need to "first configure columns (or not)" before setting the lines setting?

[snide]

and wonder if we should just try going without it for the beginning, and figure out if we're wanting to do that later if we're getting feedback from users

Agree with this approach. Also 👍 to @ryankeairns' core design.

[ryankeairns]

Thanks Tim! I don't personally have any concerns with that approach either. Also, it's good for us to get into the habit of documenting our reasoning behind such decisions, so thanks for your take.

For clarity's sake, the survey responses - while limited - did lean in the direction of applying the number of lines setting to both the default and custom (i.e. columns-added) view. I would have been more pro-switch if this were drastically flipped in the other direction.

[miukimiu]

As per my comment in #5080 (comment):

What is the "default view only"?

So I agree with @timroes that users won't know what "default view" means and we should just try going without it for the beginning.

For the design, I personally don't like toggle buttons when we have plenty of space inside the popover. I would go to something like this.

But in case we all agree to go to what @ryankeairns is envisioning, I just noticed that we can't have the "auto" inside a number field. So we would need to keep the number disabled.

Any of the options are fine for me.

[ryankeairns]

@miukimiu we could also switch the number input to a disabled text input, yeah?

In this case, I feel the labels in the button group (from your comment) become less clear and their relationship to the input below it might be less strong. We may have used this pattern elsewhere but, in this case, it seems with the small number input we can fit it all on one line, keep the tighter label to input relationship, and benefit from a more action-oriented message on the button.

[miukimiu]

we could also switch the number input to a disabled text input, yeah?

Sounds good @ryankeairns! 👍🏽

@constancecchen we have a final design.

[cee-chen]

Just want to double check something: does it sound right to everyone that the toolbar config should inherit the defaultHeight from the rowHeightsOptions configuration (if it exists)? So if the developer chooses to start the table at defaultHeight: 'auto', the toolbar button should say "Auto row height" to begin with, but if defaultHeight is undefined, then it will show the default "Row height" text - does that sound good to everyone?

Also, the current mocks indicate the toolbar button should say "Row height of 3", but IIRC we're moving to "Lines per row" instead - any objection if I change the toolbar button text to 1 line per row/3 lines per row/etc.?

[snide]

@constancecchen sounds reasonable.

[cchaos]

@constancecchen Showcased the state of this configuration in our weekly where we talked about trying to combine this setting with the density setting as to not overload the toolbar with individual options. Here's a mock/prototype for how we could achieve this. I also updated the language and the "toggle" portion for this to be a bit more clear to the user what is going to happen.

https://www.figma.com/proto/RzfYLj2xmH9K7gQtbSKygn/Elastic-UI?page-id=15895%3A196207&node-id=21744%3A266749&viewport=323%2C48%2C0.79&scaling=min-zoom&starting-point-node-id=21744%3A266749

Static:

Where the Lines per row option only shows if Wrap text is set to By line.

[ryankeairns]

Combining this with density feels related. I don't feel this new wording is as clear as the prior suggestion, however. If we're thinking of JSON documents, for example, wrapping is not the term that comes to mind. I personally prefer the Lines per row with Auto vs Number of lines toggle.

[cchaos]

I'm steering us away from using the terms Auto and Manual because they're very ambiguous. "Wrap text" is a well known term and will be understood by everyone even if it's not fully applicable to every schema type. Though I disagree that JSON doesn't have the notion of "wrapping" because it's code-centric and by default, when wrapping is off it collapses all whitespace essentially turning wrapping off.

We also encountered technical problems having only a single toggle and needed this third "Off" option to remove all wrapping and set it back to default single line truncation. I'm not 100% on "By line" but "Fit height" is also another well understood term and less ambiguous than "Auto".

[shaunmcgough]

Perhaps we should loop in @gchaps or another docs team member to focus on the microcopy. I tend to like "Auto fit row height". We also likely could have inline or popup helper text here for some of these choices. Overall, this is going to be a real win and we are on the right track from a product perspective.

[ryankeairns]

Perhaps the label could be 'Row height' and the button group options are 'Single' 'Auto fit'/'Fit height' 'Custom'
That then reads, in conjunction with the label ,as 'Single row height, Auto fit row height' and 'Custom row height'.

[cee-chen]

Just to check, should I also be moving the full-screen button and density/appearance button all the way over to the right as part of this work? I'm assuming I shouldn't be adding the Showing 1-10 of 45 Things copy?

[cee-chen]

@cchaos Just wanted to get your 2c on this - the current config for the toolbar's density toggle is called showStyleSelector (because it affects the gridStyles prop/API). Adding row height toggles to it (which is not related to gridStyles, but instead to rowHeightsOptions) should change the meaning/name of the selector just IMO.

I'm thinking something like showDisplaySelector instead of showStyleSelector. What do you think? Should we deprecate the old name of the toolbar control in favor of a new combined one?

[cchaos]

My guess is that showStyleSelector should actually still maintain the visibility of the density options but at the lower, popover contents level. Then add another prop like showRowHeightsSelector for showing this new configuration still at the popover level. Then pull up the logic as to wether to show the popover/toolbar button at all based on one of those being true. Something like..

const { showStyleSelector, showRowHeightsSelector } = this.props;

if (showStyleSelector || showRowHeightsSelector) {
  render (
    <EuiPopover button={iconButton}>
      {showStyleSelector && styleSelector}
      {showRowHeightsSelector && rowHeightsSelector}
    </EuiPopover>
  )
}

[cee-chen]

Unfortunately we already have an API in place for a toolbar control that has multiple options in it, and that's not quite the syntax it uses:

eui/src/components/datagrid/data_grid_types.ts

Lines 580 to 589 in 788c822

	export interface EuiDataGridToolBarVisibilityColumnSelectorOptions {
	/**
	* When `false`, removes the ability to show & hide columns through the UI
	*/
	allowHide?: boolean;
	/**
	* When `false`, removes the ability to re-order columns through the UI
	*/
	allowReorder?: boolean;
	}

My preference would be to follow a nested obj syntax like the above for more consistent configuration vs. 2 separate APIs.

[cchaos]

Sorry I'm not at all familiar with what exists today, so I'm purely basing my comment on the context provided in your question. My code was mostly an example of keeping the row heights selector and the density selector as two separate options that can be turned on/off individually.

[cee-chen]

Right, we do want those still to be able to be turned off individually, but I also think it makes sense from a consumer standpoint to also be able to turn off the entire icon/control with just a single false rather than 2. If no one has any objection to deprecating showStyleSelector, I'll roll that into the PR as well.

Just curious also, where should custom additional controls land in your above mock @cchaos? To the left of the columns/sort actions or to the right?

[cchaos]

To the left of the columns/sort actions or to the right?

I would think at some point we might want that to be configurable, but for now since it isn't, to the right.

[cee-chen]

Sounds good! As a heads up that is a change from our current code, despite what our props description say it looks like we render custom additionalControls to the left of all other buttons 🤷 I'll update our props documentation as well when I'm doing this work!