[Emotion] Convert final EuiDataGrid cell styles (Part 5) #8013
Conversation
cee-chen
force-pushed
the
emotion/datagrid-4
branch
2 times, most recently
from
323aaf9
to
a46b499
Compare
+ flatten specificity with `:where` selector
- JS logic lets us flatten and add more explanatory comments for the different height types
cee-chen
force-pushed
the
emotion/datagrid-4
branch
2 times, most recently
from
fce624a
to
44a3b6c
Compare
- fairly straightforward - DRY out flex CSS between cell and focus trap div
- just inline it, it's only used once
- flatten left/right CSS since we have the prop right here + remove unnecessary flex-shrink CSS on the arrow - `EuiIcon` already takes care of that inherently
- just reuse EuiButtonIcon with transform/animations unset and remove `__icon` - simplifies a ton of CSS - handle fontSize sizing of actions button in top level data grid styles - use `:not()` CSS to handle hidden state instead, which simplifies even more CSS
+ convert inline Emotion to styles & fix line-height shenanigans + move complex selectors to new util + wrap transition in animation media query
- remove unnecessary `.euiDataGridHeaderCell__content` div element in control columns - it's not doing anything useful, so we might as well delete it + fix `focus_utils` rendering a `null` in control column `aria-describedby`
- font weight appears to be unused from an old theme - let's replace it with actual english text instead that indicates what clicking the button will do
+ [enhancement] improve smoothness of dragging visibility with an `isDragging` modifier - clean up unnecessary inline style (only useful when dragging, we don't need it during, e.g. snapshots) + convert enzyme tests to RTL (requires using a ref for class methods instead of `.instance()`)
cee-chen
force-pushed
the
emotion/datagrid-4
branch
from
44a3b6c
to
95432af
Compare
…lbarVisibility` stories - for some reason the ref was breaking the docgen?? - we didn't actually need the stateful grid for the ref tests, so just get rid of it
cee-chen
force-pushed
the
emotion/datagrid-4
branch
from
95432af
to
456f5ef
Compare
There was a problem hiding this comment.
Overall the changes look great! I noticed 2 smaller regressions:
- the cell height is slightly increase (by
2px) - this might be intentional, but I couldn't really tell what caused it.
Screen.Recording.2024-09-11.at.12.42.03.mov
- the action button min. target size is not reached for
fontSize="s"(see standalone comment)
This is not a regression in any styles, it's a change in the Storybook arguments (noticeable also by the way the default storybook was not correctly truncating text). This was the previous setting for eui/packages/eui/src/components/datagrid/data_grid.stories.tsx Lines 300 to 301 in 908a718 However, this is not actually correct/equivalent to an undefined eui/packages/eui/src/components/datagrid/data_grid.stories.utils.tsx Lines 286 to 287 in d7d70c2 Defining a static height is what's causing the aforementioned lack of text truncation. Additionally, the cell borders (2px total) are considered "included" in any height passed to us, thus why the cells are 2px shorter. The |
…ction component - not sure how much performance this is gaining us (if at all, it might be losing?) so IMO this is primarily a dev readability enhancement + make a few more props not overrideable via `setCellProps` (& update types) + write tests for merged `setCellProps` props
- otherwise, it continuously reads out "and 3 more items" instead of the full SR text :/
- neither the SR text or the cell actions are dependent on the renderCellValue content, and they don't need to be parsed by `HandleInteractiveChildren`, so move them out to the parent EuiDataGridCell + clean up now-unnecessary waterfalled props - move screen reader text out to its own function component to continue using useEuiI18n
cleaner snapshots, less DOM rendered, and and behaves the same way for screen readers, so why not
- aria-describedby text doesn't need to be visible at all, we can just use `hidden`, which removes it from the copy clipboard + condense down to a single element
|
@mgadewoll I was spiking out some copy shenanigans with EuiDataGrid's copy behavior and noticed some more cleanup/refactor opportunities that I've pushed up to this branch. In particular, I'd love your help QA testing 0b29898 to ensure that it works as before/correctly on JAWS/NVDA! |
💚 Build Succeeded
History
|
packages/eui/src/components/datagrid/body/cell/data_grid_cell.tsx
Outdated
Show resolved
Hide resolved
|
Thank you so much for the thorough testing Lene!!!! ❤️ You rock! |
Summary
closes #6394 and #5685 😭 🥳 🎉 🎊 🍾
This PR additionally includes some EuiDataGridHeaderCell cleanups from #7898 (see individual commits tagged with
[refactor]), and some misc tech debt at the end of the commits.Don't be scared by the files changed count, that's almost entirely loki VRT screenshots that I re-ran to update final differences. After this point, EuiDataGrid should be fully up to date and no screenshots should rerender differently locally 🎉
QA
General checklist
screenreader modesreduced motion mode- [ ] Checked in mobileAdded orupdated jestand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)