[EuiDataGrid] Cell actions cleanup + bug fixes #5592
Conversation
- to be more specific and intentional about what we're referring to
- `__expandButtonIcon` is redundant with `__actionButtonIcon` - DRY it out and have the expand popover action use `__actionButtonIcon` - isActive CSS is no longer used whatsoever - remove conditional CSS and need for `popoverIsOpen` prop - Rename animation / last cellButton reference to cellActions
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5592/ |
- remove `:hover:not(:focus)` in favor of simply having `:focus` disable animations, which lets us DRY out CSS with `.euiDataGridRowCell--open` - add `:focus-within` catch that prevents flickering animation issue after cell popover expansion close - (lint) remove unnecessary brackets around `euiDataGridRowCell--open` class
`border` in particular was causing the cell slide in animation to look wonky - this is worth applying to all cell action button icons, not just the cell expansion popover, since it will also fix issues for custom actions that have `display="fill"`
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5592/ |
There was a problem hiding this comment.
Changes LGTM; tested the PR preview and confirm the changes are in place, and cannot break other UI interactions.
However, the tradeoff is that the cell actions show up instantly without a slide in animation if a user is really fast about hovering & immediately clicking into the cell (which is arguably still a better UX than the animation replaying on every hover)
💯 I think that's even better
I've added @miukimiu as a reviewer 'cuz that's too much CSS change for me to feel ok calling good
|
@miukimiu Just discovered that this affects my upcoming work for cell popover customization, so I'd super love to get this merged in sooner rather than later if possible! 🙇♀️ |
There was a problem hiding this comment.
Thanks, @constancecchen! LGTM! 🎉
Tested both fixes in Chrome, Firefox and Safari. I also looked at the SCSS changes.
|
Thanks a million!!! |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5592/ |
Summary
This PR is part cleanup and 2 separate bugfixes.
Cleanup:
expandButtonIcon/actionButtonIconCSSBug 1: c1f3ce8 & 8d1d0aa
Setting both
isExpandable: falseandcellActions: [a, b, c]would not correctly disable/hide the cell expansion action button. 8d1d0aa fixes that behavior and conditionally renders the expand button only ifisExpandableis true.Bug 2: 969e3a2 & f8f1279
closes #4960
This is not a perfect fix to the linked issue, but I think it's most straightforward one. Cell actions will no longer perform the slide in animation when mousing out and back in to the cell when it's already been clicked. However, the tradeoff is that the cell actions show up instantly without a slide in animation if a user is really fast about hovering & immediately clicking into the cell (which is arguably still a better UX than the animation replaying on every hover).
Checklist
- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes