[Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) #8011

cee-chen · 2024-09-09T18:30:58Z

Summary

This PR:

Converts cell popover, actions, and focus outline to Emotion ([Emotion] EuiDataGrid #6394)
Addresses some of [EuiDataGrid] Cell filtering UX unusable after scroll #7828 (comment) (marked in individual commits)
Fixes a bug with EuiDataGridCell not correctly merging data-test-subjs

As always, I recommend following along by commit. The last commit/refactor is a bit spicy - I'd love to hear feedback on whether y'all feel it helps make the CSS more understandable & maintainable or if it does the opposite 🫠

QA

Go to https://eui.elastic.co/pr_8011/#/tabular-content/data-grid or https://eui.elastic.co/pr_8011/storybook/?path=/story/tabular-content-euidatagrid--playground
- On hover (but not click), confirm that the cell outline+actions are gray and the cell actions have a slight delay before showing up
- On click, confirm that the cell outline+actions are blue, and cell actions show up immediately
- On keyboard arrow navigation of cells, confirm that cell actions show up immediately and outline+actions are always blue
New hover zone affordance testing
- On hover (without clicking), move your mouse diagonally from the cell content to the actions and confirm that there is more buffer/affordance for this compared to production
Go to your OS system accessibility settings and toggle Reduce motion
- Confirm cell actions show up immediately on hover
- Confirm cell popovers show with no transition/animation on open
Header cell outline testing - go to Storybook or focus docs
- Confirm no outline shows on hover
- Confirm the focus outline shows on click
- Confirm the focus outline shows when the cell actions is opened both via keyboard enter and mouse click

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only ~~and screenreader modes~~
  - [ ] Checked in mobile
Docs site QA - N/A
Code quality checklist
- Added or updated jest and cypress tests
- Added visual regression tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

- per recent Shared UX request

+ remove CSS `filter: none` unset - `attachToAnchor` prop handles that already + refactor `!important` z-index CSS to use JS var and popover prop instead + remove enzyme snapshots, refactor shallow-like test snapshot with Emotion cruft to render DOM instead

- remove :focus-within - it was causing bugs and is no longer necessary with new cell actions design + remove enzyme snapshots

+ start row/header cell styles, but leaving the rest of that CSS for later - move away from CSS var to Emotion util, it's easier when separating styles by component/concern + fix/tweak cell actions CSS to account for new setup

- remove unnecessary close/reopen popover Cypress test - remove unneeded realClick/waits with realMount

- instead of snapshots + fix EuiDataGridCell not correctly merging `data-test-subj` 🤦

- hooray for data grid's ref API allowing us to show cell actions and popovers without `play()`!

- to reduce unnecessary selectors - to comment everything in one place + write E2E tests checking for expected CSS behavior + add UI enhancement that shows the outline when cell focus traps are entered

mgadewoll · 2024-09-10T12:50:59Z

packages/eui/src/components/datagrid/body/cell/data_grid_cell_actions.styles.ts

+    `,
+    visibility: css`
+      /* If a cell is not hovered nor focused nor open via popover, don't show the actions */
+      .euiDataGridRowCell:not(:hover, ${hasFocus}) & {


That's a more clean selector now 🧹

mgadewoll · 2024-09-10T13:04:33Z

packages/eui/src/components/datagrid/body/cell/data_grid_cell_actions.styles.ts

+      /* Increase non-visible hitbox of cell on hover, to reduce UX friction
+       * for users mousing from the cell diagonally over to the actions */
+      .euiDataGridRowCell:hover:not(${cellOutline.rowCellFocusSelectors}) & {
+        ${logicalCSS('min-width', '50%')}


I'm wondering if this is too much for single action popovers and might cause confusion? 🤔
Maybe the padding alone would be enough?

My 2c is that it's not about the cell actions, it's about the cell itself. Users are likely to mouse over from the cell to the actions, so we should inherit the hitbox/hover zone from the cell width.

Example mouse path of what I mean - if a user's mouse is at the start of the arrow and they move in a straight diagonal line to the cell action, then the 50% width hover zone is more likely to capture that than a static padding:

Yeah, I understand. And thinking about it further it's probably better to try with more space and worst case we reduce it if there are issues 👍

mgadewoll · 2024-09-10T13:20:14Z

packages/eui/src/components/datagrid/body/cell/data_grid_cell.styles.ts

+  };
+};
+
+export const euiDataGridCellOutlineSelectors = (parentSelector = '&') => {


This is "more code" but I personally like and prefer this refactor. ✨
I was a bit confused by the previous usage of rowCellFocusSelectors as it abstracted away some selectors. Now the usage is more clear as each variable has a clear purpose 👍

Super glad to hear that!! 🎉

mgadewoll · 2024-09-10T13:20:30Z

packages/eui/src/components/datagrid/body/cell/data_grid_cell.styles.ts

+  const headerActionsOpen = '.euiDataGridHeaderCell--isActionsPopoverOpen';
+
+  // Utils
+  const selectors = (...args: string[]) => [...args].join(', ');


The only thing I'm not fully sold on is if this selectors() is really needed?
If I understand it correctly the difference is this? If so, it's not really that much less to write 🤔

selectors(hover, focus, isOpen, isEntered) vs `${hover}, ${focus}, ${isOpen}, ${isEntered}`

It's very minor but I personally find it easier to read the first API / it's easier for my brain to see the list of states without the extra ${} syntax! It's also fewer characters, so Prettier is less likely to send it down to multiple lines which is sometimes annoying.

I guess I understand why it can be easier to perceive without ${}, I guess I'm just too used to filter them out 😅
I don't have a hard opinion on it, I'm fine either way.

needs moar pixels

mgadewoll

🚢 🐈‍⬛ The updates work great, I didn't see any visual regression.
Kudos for the cleanups and refactors to make the code easier to grasp!

- actions ::after position was slightly off, I fixed this after taking the first screenshots

cee-chen · 2024-09-10T16:34:23Z

Thank you so much as always for the PR reviews Lene! I'm going to throw a huge party after the final PR 😭

elasticmachine · 2024-09-10T17:00:27Z

💚 Build Succeeded

Buildkite Build
Commit: fb28e29

History

💔 Build #2715 failed f3fab78
💚 Build #2712 succeeded 01dbd90
💚 Build #2711 succeeded 6939bf2
💚 Build #2710 succeeded e63eab6
💔 Build #2709 failed dbd3ef1

cee-chen added data grid emotion labels Sep 9, 2024

[7828] Remove dynamic horizontal alignment of cell actions/cell popover

0171433

- per recent Shared UX request

cee-chen force-pushed the emotion/datagrid-2 branch 2 times, most recently from e63eab6 to 6939bf2 Compare September 9, 2024 23:54

cee-chen changed the title ~~[Emotion] Convert remaining EuiDataGrid row/footer/header cell styles (+ cell actions/outline) (Part 4)~~ [Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) Sep 9, 2024

cee-chen force-pushed the emotion/datagrid-2 branch 2 times, most recently from 3110dd0 to f3fab78 Compare September 10, 2024 06:20

cee-chen added 10 commits September 9, 2024 23:23

Convert cell actions styles

602a967

- remove :focus-within - it was causing bugs and is no longer necessary with new cell actions design + remove enzyme snapshots

Convert cell focus outline styles

e2628e6

+ start row/header cell styles, but leaving the rest of that CSS for later - move away from CSS var to Emotion util, it's easier when separating styles by component/concern + fix/tweak cell actions CSS to account for new setup

[7828] Increase hover zone of cell actions per feature request

bacf992

Fix/harden remaining flaky Cypress tests

011ba49

- remove unnecessary close/reopen popover Cypress test - remove unneeded realClick/waits with realMount

Fix/improve failing Enzyme tests to use RTL + specific assertions

65add0c

- instead of snapshots + fix EuiDataGridCell not correctly merging `data-test-subj` 🤦

Update downstream snapshots

5c5bad5

Add VRT tests

b221857

- hooray for data grid's ref API allowing us to show cell actions and popovers without `play()`!

changelog

47b1b18

[refactor] Rewrite focus/hover selectors

75cf399

- to reduce unnecessary selectors - to comment everything in one place + write E2E tests checking for expected CSS behavior + add UI enhancement that shows the outline when cell focus traps are entered

cee-chen marked this pull request as ready for review September 10, 2024 06:38

cee-chen requested a review from a team as a code owner September 10, 2024 06:38

mgadewoll reviewed Sep 10, 2024

View reviewed changes

Fix failing Cypress test

9a0f296

needs moar pixels

cee-chen force-pushed the emotion/datagrid-2 branch from f3fab78 to 9a0f296 Compare September 10, 2024 16:22

mgadewoll approved these changes Sep 10, 2024

View reviewed changes

Final VRT update

fb28e29

- actions ::after position was slightly off, I fixed this after taking the first screenshots

cee-chen enabled auto-merge (squash) September 10, 2024 16:34

cee-chen merged commit 4308a65 into elastic:main Sep 10, 2024
5 checks passed

cee-chen deleted the emotion/datagrid-2 branch September 10, 2024 17:00

This was referenced Sep 10, 2024

[EuiDataGrid] Cell filtering UX unusable after scroll #7828

Open

Upgrade EUI to v95.11.0 elastic/kibana#192756

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) #8011

[Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) #8011

cee-chen commented Sep 9, 2024 •

edited by mgadewoll

Loading

mgadewoll Sep 10, 2024

mgadewoll Sep 10, 2024

cee-chen Sep 10, 2024

cee-chen Sep 10, 2024

mgadewoll Sep 10, 2024

mgadewoll Sep 10, 2024

cee-chen Sep 10, 2024

mgadewoll Sep 10, 2024 •

edited

Loading

cee-chen Sep 10, 2024

mgadewoll Sep 10, 2024

mgadewoll left a comment

cee-chen commented Sep 10, 2024

elasticmachine commented Sep 10, 2024

[Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) #8011

[Emotion] Convert EuiDataGrid cell popover, actions, and focus outline styles (Part 4) #8011

Conversation

cee-chen commented Sep 9, 2024 • edited by mgadewoll Loading

Summary

QA

General checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgadewoll Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgadewoll left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 10, 2024

elasticmachine commented Sep 10, 2024

💚 Build Succeeded

History

cee-chen commented Sep 9, 2024 •

edited by mgadewoll

Loading

mgadewoll Sep 10, 2024 •

edited

Loading