[EuiDataGrid] Enforce visibleCellActions shown (overflowing extra actions to the cell popover)

[cee-chen]

Summary

closes #5132

Cell action buttons on the cell will only show a maximum number of actions on the cell (default limit is 2, but this is configurable via columns.visibleCellActions). The remaining items in the columns.cellActions array will only be displayed on the cell expansion popover, in a second column-style footer.

Screencap

QA

https://eui.elastic.co/pr_5675/#/tabular-content/data-grid-cell-popovers#cell-actions-and-cell-popovers

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles

• Added documentation

- [ ] Checked Code Sandbox works for any docs examples

• Added or updated jest and cypress tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[snide]

Yes! This feels so tidy! Likely this will be a breaking change in Security, who made some hacks to make this work in the alerting view. We'll want to let them know it's coming.

cc @monina-n and @mdefazio on the design side as well.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[chandlerprall]

Made a small suggestion, but everything else looks good. Couldn't find any issues in the preview build, and those new tests are great!

[chandlerprall]

Approved, but let's hold off on merging until we can coordinate with the security folks as Dave mentioned (I've reached out to Andrew)

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[cchaos]

Ok, I've just been doing some thinking around the design (mainly treatment of the buttons in the popover) aspect of this thing. The demos look great because everything is actually pretty lined up well based on the content chosen. But I have some concerns with the "raggedy-ness" that happens with stacks of centered buttons. Specifically:

A. Choosing more inconsistent labels showcases the ragged reading margins
B. If the consumer decides the first 2 primary aren't the paired ones, there's still a raggedness and the reading order odd
C. If we decide just to stack them all, both are even far more an issue
D. We could just stack it all and align them left (like our context menus). This one makes the most sense/easiest to provide but we do lose the nicety we get with being able to pair a common pattern of filter in/out.

So... with that, a solution could be:

E. Keep the first two in a row, but just left align the subsequent buttons.
F. But allow the consumer to ask to "stack" the primary actions.

[cee-chen]

E was indeed @mdefazio's original mock, but the buttons looked really weird in terms of click/hover areas with alignItems="flexStart":

It wasn't clear to me whether or not the buttons should have been intentionally full width or not, and the missing hitbox felt annoying to me UX-wise - I also thought centering looked less weird especially on cells with very long content - see below example:

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[cchaos]

Yep, I agree with all your concerns, but I think the (un)readability of centered content outweighs them. My suggestion would be to use EuiContextMenuItems in place of EuiEmptyButtons. But it might require adding support for color.

[cee-chen]

My suggestion would be to use EuiContextMenuItems in place of EuiEmptyButtons

This would convolute our API/typing a ton unfortunately, since the way we handle cellActions is we tell consumers we're passing them a component that will either be an EuiButtonIcon (if on the cell) or an EuiButtonEmpty (if in the popover), and consumers render that component with whatever button props they want. At a glance there's a whole bunch of diffferent props between EuiButton* and EuiContextMenuIcon, so it would likely be a mess. it would be much simpler to customize the styling of the popover buttons to look more like what we want them to than change the underlying component.

For now I've simply added alignItems="flexStart" onto the secondary actions footer, feel free to play around with it

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[cchaos]

I've pushed you a simple change to the buttons in which I wrap each button (primary and secondary) in a <div> so they don't expand within the flex items.

Reasoning:

These buttons are EuiEmptyButtons, so they don't have visible bounds unless they're focused. When they expand, they cause white space to be clickable (as shown in your gifs). While this seems off compared to our context menus, they are more likely to cause mis-clicks because of how wide the popover can get compared to the link text.

Restricting the button's interactive area to just the text/icon is less likely to cause mis-clicks and create intentionality.

The then lines of the left end of the primary and secondary buttons so we don't have the weird layout.

Examples:

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[MikePaquette]

Sorry to join this conversation late, but I'm wondering if/why we need to enforce a maximum of 2 items to be displayed?

During the RAC project, as we adopted the EUI data grid for alert table use, we agreed that the EUI default display of two items made sense, but our understanding was that an optional addition of another item would be acceptable if the impact to the overall width was nominal and there was good justification from the use case perspective. Why are we now changing this at the expense of a breaking change to the security solution?

In the security solution, the ability to add any ECS field displayed in a cell to the currently active timeline investigative workspace is a key differentiator that allows both threat hunter and SIEM analyst personas to easily start or continue an investigation. By forcing users to perform another click of the overflow icon to add the item to a Timeline, we are reducing the discoverability of this key feature, and adding another click for each item they want to add to the timeline, which will decrease their velocity. This is in direct conflict with one of our primary objectives, which is to increase analyst velocity during investigations so that we can help them reduce their mean-time-to-detect MTTD and mean-time-to-respond MTTR metrics.

The security usage includes 3 items (we use 5 in our non-EUI tables) in the display today, which seems to widen the display only minimally, like this:

Can we please reconsider and revert the "enforce" nature of this PR so as to not negatively impact our security solution users, and/or not burden security solution developers with extra work to build an alternate solution?

cc: @monina-n @adamwdraper @paulewing @oatkiller @chandlerprall @snide

[snide]

@MikePaquette I'm going to set up a zoom invite your way. There's no immediate hurry to merge this PR and we have time to talk it through.

[mchopda]

@snide @MikePaquette I am interested in knowing the summary of the zoom meeting as I agree limiting the cell actions may not satisfy security use cases.

[cchaos]

This PR is simply the implementation from feature request that was submitted back in September 2021: #5132. The EUI issue was created after lengthy discussion between designers on the current UX implications of having too many cell actions. You can further trace discussions by checking out the linked issues in #5132. If there was a breakdown in communication between solution needs and design expectations, I suggest we table this PR (put it to draft) and I'd like to refer everyone back to the originating issue to continue discussion.

[snide]

OK. I think we have a consolidated front from design, product and engineering on how to move this PR forward. I've written up some prop changes here #5132 (comment) which themselves came from discussion #5089 (reply in thread) .

Further. We've agreed to hold this PR to merge for two weeks until 8.2 feature freeze to give solution teams adequate time to make changes.

@constancecchen or anyone else on this thread feel free to bug me if those discussions and decisions are unclear. Otherwise I think we have a solid path to merge this tidy bit of code! Thanks for everyone's patience.

[cee-chen]

New code with columns.visibleCellActions API has been pushed up and is ready for review. Screencap:

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5675/

[chandlerprall]

All changes LGTM; tested the column.visibleCellActions change in the preview docs and it works as expected

[cee-chen]

Hey, just a quick ping to everyone who's commented on this PR so far. 8.2FF is tomorrow so I'll likely be merging this PR in sometime this week. Does anyone see any issues with the user or developer experience of https://eui.elastic.co/pr_5675/#/tabular-content/data-grid-cells-popovers#visible-cell-actions-and-cell-popovers? If so, please speak up sooner rather than later 😃

[cee-chen]

Going to go ahead and merge this, as 8.2 FF is past.