EuiDataGrid cellAction and expand popover

[mdefazio]

Request for update to options for EuiDataGrid cellAction

Reason for change

As more Solutions are using the EuiDataGrid for a wider range of use cases, we should consider baking in some guidelines to the component alongside best practices so we can remain consistent in our usage across Kibana.

Proposed change

One of our recommendations is that no more than 3 icons should be visible on hover (including the expand action). However, we have several use cases where additional actions are required.

Currently, it is possible to add as many actions as desired. However, we should follow a similar rationale as the EuiBasicTable.

There can only be up to 2 actions visible per row. When more than two actions are defined, the first 2 isPrimary actions will stay visible, an ellipses icon button will hold all actions in a single popover.

For the purposes of the EuiDataGrid, the ellipses icon is replaced with the expand icon on a cell.

Example:

---

Implementation:

• distinguish between primary & non-primary actions
• limit the cell actions to the icons for 0...2 primary actions + expansion icon
• provide the actions & their grouping (primary vs. non) to the default popover, and render accordingly
• provide the actions & their grouping (primary vs. non) to custom popovers
  • maybe also provide a React element that already has a ready-to-use structure, re-used from the default popover?

So with this in mind, we can (somehow) mark actions as primary and display up to the first 2 actions as the exposed hover actions. Within the expand popover, display these same actions in a horizontal row (current display option). Non-primary actions would not be visible on hover within the cell, and display in a vertical list below the primary actions row within the popover.

If no primary actions are found, then no actions (aside from expand) are shown on hover and all actions are displayed as a list within the popover.

[chandlerprall]

We're currently focusing on the row height selector (#5080) for the Discover team, but will pick up these cell action patterns & changes next.

[cee-chen]

Hey @mdefazio! I'm spiking this out currently and wanted to ask you about the express need for some kind of way to mark primary actions. Since we're passing a direct array of components to columns.cellActions and not an object config, there's no quick or easy way to mark actions with some kind of isPrimary flag.

Can we instead opt to use array order as the logic for truncating actions? So the first two actions are always shown/primary, and all actions after that are considered secondary and only show up in the expansion popover? I think "the first two actions are primary" is honestly more straightforward, unless I'm missing some context/a use-case.

[mdefazio]

@constancecchen Yes, I think that's absolutely reasonable. We discussed defaulting to doing it this way previously and I agree it makes it more straightforward. I believe the isPrimary was a remnant from previous iterations and just stuck around when we talked about it.

[cee-chen]

@mdefazio @chandlerprall

One more quick implementation question I just came across: if a consumer has more than 2 actions but sets isExpandable: false in their column/cell configuration (which turns off the cell popover), this means their extra actions are no longer accessible. Do we need to bake in an extra edge case for this? So if there are more than 2 cell actions, but isExpandable is false, we show a custom popover with no cell content but with all cell actions?

Or do we assume that consumers should know what they're doing, and if they deliberately turn off/set isExpandable to false, they know that they're turning off access to more than 2 actions?

[cee-chen]

Ah, actually, I think I may have mistakenly broken this behavior in #5592. Based on our props documentation of isExpandable, it looks like the expansion cell popover should always show if cellActions is defined. That would solve the above UX issue.

Any objections if I change our internal code accordingly and add more documentation to note that adding cellActions will always force an expansion popover?

[chandlerprall]

Any objections if I change our internal code accordingly and add more documentation to note that adding cellActions will always force an expansion popover?

Sounds right to me 👍

[cee-chen]

@MikePaquette @mchopda - tagging you in on this issue as it's the one to discuss design decisions on. @mdefazio, any chance you can help out here and address Security's concerns about the 2 cellActions limit? I believe they currently show/want 3 on Security data grids.

[mdefazio]

Reopened our previous discussion.

[snide]

A small note to the above that requirements were added to add a prop to the data-grid configuration as discussed here #5089 (reply in thread) . Essentially the request is to add a visibleCellActions={2} prop that defines the amount of actions that appear in cells within a particular column. Default would be 2, with allowance for larger numbers. The overflow of additional actions into the popover would adjust as defined above past that number.

A separate, strong suggestion is that width adjustable columns should have a set minimum width, with an addition pixel value for each visibleCellAction.

These suggestions are pseudo-code. If the EUI team has a better manner to name / make this functionality available feel free to adjust. Similar with any suggestions of how to calculate minimum width of columns.