[Emotion] Convert `EuiDroppable` & `EuiDraggable` #7187

cee-chen · 2023-09-14T02:51:00Z

Summary

As always, I recommend following along by commit as this PR contains some cleanup/tech debt tasks around tests etc.

This PR additionally contains a very small focus outline placement fix in 6e1823e:

Before:

After:

QA

Go to https://eui.elastic.co/pr_7187/#/display/drag-and-drop
Smoke test each docs example and confirm display is the same as production
Go to https://eui.elastic.co/pr_7187/#/tabular-content/data-grid
Smoke test the columns and sorting toolbar popovers (add a sort to test dragging/dropping sort order) and confirm they work as expected (you should be able to drag the handle icons and the column titles, but not interactive elements such as switches)

General checklist

Browser QA
~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA - skipped
Code quality checklist
- Added or updated jest ~~and cypress 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~
- ~~[ ] Updated the Figma library counterpart~~

Emotion checklist

Kibana usage

Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes - Two modifier usages: 1, 2
Any test/query selectors that will need to be updated - None
Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted - None
Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component) - None

General

Output CSS matches the previous CSS (works as expected in all browsers)
Rendered className(s) read as expected in snapshots and browsers
~~[ ] Checked component playground~~ No playground, but added Storybooks

Unit tests

shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
Enzyme tests converted to RTL

Sass/Emotion conversion process

Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
Removed ~~or converted~~ component-specific Sass vars/mixins ~~to exported JS versions~~
Listed var/mixin removals in changelog
~~[ ] Ran yarn compile-scss to update var/mixin JSON files~~
~~[ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~
~~[ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~
All SCSS files have been removed from the component(s) directory
~~[ ] All SCSS overrides have been removed from the Amsterdam theme~~ No overrides

CSS tech debt

Wrapped all animations or transitions in euiCanAnimate
~~[ ] Used gap property to add margin between items if using flex~~
~~[ ] Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)~~

DOM Cleanup

Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child) - Note: removed an unused modifier class on a child euiDroppable__placeholder element
SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Extras/nice-to-have

Reduced specificity where possible (usually by reducing nesting and class name chaining)
~~[ ] Documentation pass~~
~~[ ] Check for issues in the backlog that could be a quick fix for that component~~
~~[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about~~

+ remove modifier classes

- remove more modifier classes - [syntax nit] destructure `snapshot` for more readable code

- no theme tokens, so can be a static obj + remove 4 year old TODO that has never been added and probably isn't needed at this point

`euiFocusRing` mixin doesn't work because of `:focus-visible` prefer just an auto outline to match prod behavior, the outline is just more focused on the child item not sure what the custom drag handle CSS is for, drag handle focus outlines work fine as-is

- Remove all unused modifier classes (based on Kibana usage) - Fix cloned items missing Emotion styles - Fix missing React 17 snapshot updates

- DRY out with EuiDraggable spacing + switch `none` to a null to match EuiPanel padding styles (reduces rendered classNames) + remove exported types, not really being used

…ribute

- convert all tests to RTL - add shouldRenderCustomStyles tests - switch modifier class to non BEM to indicate state vs styles

- add `shouldRenderCustomStyles` test - DRY out test context wrapper logic - fixed `...requiredProps` logic - switch from screen usage to destructured APIs

- remove unnecessary/unused jest mock

+ fix `false` className by using `classNames()` util

- extra selector specifier needed due to new Emotion wrapper around drag/drop components

cee-chen · 2023-09-15T00:54:21Z

@breehall I opted not to create a Storybook for this conversion mostly for expedience - the existing EUI docs are quite good for QA, and the props are a little tricky since this is a very 3rd party lib heavy and render prop heavy component. I'd love to see you create stories for drag/drop in the future however! (no rush, of course)

kibanamachine · 2023-09-15T01:19:00Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7187/
Storybook: https://eui.elastic.co/pr_7187/storybook

elasticmachine · 2023-09-15T01:24:06Z

💚 Build Succeeded

Buildkite Build
Commit: 283792a

History

💚 Build #622 succeeded 5342a6d
💚 Build #610 succeeded c9843c3

breehall

Great conversion! It was actually a little less complicated than I thought it would be. I thoroughly tested the QA against prod and didn't find any unexpected changes.

One question on a class name below, but I think that's about it!

src/components/drag_and_drop/draggable.styles.ts

breehall · 2023-09-18T15:33:33Z

src/components/drag_and_drop/droppable.tsx

+        const PanelOrDiv = withPanel ? EuiPanel : 'div';
+        const panelOrDivProps = withPanel
+          ? {
+              panelRef: provided.innerRef,
+              hasShadow: true,
+              paddingSize: 'none' as const,
+            }
+          : { ref: provided.innerRef };
+


Elegant refactor and use down on line 119!

breehall · 2023-09-18T15:37:35Z

src/components/drag_and_drop/droppable.tsx

-            'euiDroppable--noGrow': !grow,
-          },
-          spacingToClassNameMap[spacing],
+          { 'euiDroppable-isDisabled': dropIsDisabled },


Are we just keeping this class for Kibana usages?

Yes, there's two Kibana usages targeting --isDisabled (that restore the cursor).

`v88.3.0`⏩`v88.5.0` closes #151514 --- ## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0) - Updated `EuiCallOut` with a new `onDismiss` prop ([#7156](elastic/eui#7156)) - Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows custom rendering of the toolbar. ([#7190](elastic/eui#7190)) - Added a new `allowResetButton` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows hiding the "Reset to default" button from the display settings popover. ([#7190](elastic/eui#7190)) - Added a new `additionalDisplaySettings` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows rendering extra settings inside the display settings popover. ([#7190](elastic/eui#7190)) - Updated `EuiDataGrid`'s toolbar display settings button icon ([#7190](elastic/eui#7190)) - Updated `EuiTextTruncate` with significantly improved iteration performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation` now only uses more performant canvas render API ([#7210](elastic/eui#7210)) - Updated `EuiPopover` with a new configurable `repositionToCrossAxis` prop ([#7211](elastic/eui#7211)) - Updated `EuiDatePicker` to support `compressed` input styling ([#7218](elastic/eui#7218)) - Added `gradient` and `palette` icon glyphs. ([#7220](elastic/eui#7220)) **Bug fixes** - Fixed `EuiPopover`'s missing animations on popover close ([#7211](elastic/eui#7211)) - Fixed `EuiInputPopover` anchoring to the wrong side and missing shadows on smaller screens ([#7211](elastic/eui#7211)) - Fixed `EuiSuperDatePicker` icon spacing on the quick select button ([#7217](elastic/eui#7217)) - Fixed a missing type in `EuiMarkdownEditor`'s default processing plugins ([#7221](elastic/eui#7221)) ## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1) **Bug fixes** - Fixed missing `className`s on `EuiTextTruncate` ([#7212](elastic/eui#7212)) - Fixed `title`s on `EuiComboBox` dropdown options to always be present ([#7212](elastic/eui#7212)) - Fixed `EuiComboBox` truncation issues when search is an empty space ([#7212](elastic/eui#7212)) ## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0) - Updated `EuiComboBox` to allow configuring text truncation behavior via `truncationProps`. These props can be set on the entire combobox as well as on on individual dropdown options. ([#7028](elastic/eui#7028)) - Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to `eql`). When setting this prop to `text`, the built-in search bar will ignore EQL syntax and allow searching for plain strings with special characters and symbols. ([#7175](elastic/eui#7175)) **Bug fixes** - `EuiComboBox` now always shows the highlighted search text, even on truncated text ([#7028](elastic/eui#7028)) - Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text ([#7175](elastic/eui#7175)) - Fixed the inline compressed styles of `EuiDescriptionListTitle` to use a taller line-height for readability ([#7185](elastic/eui#7185)) - Fixed `EuiComboBox` to correctly truncate selected items when displayed as pills and plain text ([#7193](elastic/eui#7193)) **Accessibility** - Added `aria-current` attribute to `EuiTablePagination` ([#7186](elastic/eui#7186)) **CSS-in-JS conversions** - Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed `$euiDragAndDropSpacing` Sass variables ([#7187](elastic/eui#7187)) --------- Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com> Co-authored-by: Jan Monschke <jan.monschke@elastic.co> Co-authored-by: Thomas Watson <watson@elastic.co>

cee-chen added 6 commits September 14, 2023 17:19

[misc cleanup] fragment syntax

22fa09b

[EuiDraggable] Set up initial Emotion styles, convert spacing styles

04322df

+ remove modifier classes

[EuiDraggable] Convert more styles to Emotion

4a3af3d

- remove more modifier classes - [syntax nit] destructure `snapshot` for more readable code

[EuiDraggable] Convert child __item styles to Emotion

538a852

- no theme tokens, so can be a static obj + remove 4 year old TODO that has never been added and probably isn't needed at this point

[EuiDraggable] Misc cleanup/fixes

bf756a7

- Remove all unused modifier classes (based on Kibana usage) - Fix cloned items missing Emotion styles - Fix missing React 17 snapshot updates

cee-chen force-pushed the emotion/drag-drop branch from c9843c3 to 5342a6d Compare September 15, 2023 00:19

cee-chen added 11 commits September 14, 2023 17:47

[EuiDroppable] Convert spacing to Emotion

4c3ab49

- DRY out with EuiDraggable spacing + switch `none` to a null to match EuiPanel padding styles (reduces rendered classNames) + remove exported types, not really being used

[EuiDroppable] Convert more Emotion styles

b46d7ef

[EuiDroppable] Convert conditional hidden placeholder logic to JS/att…

62c6f27

…ribute

[EuiDroppable] Convert euiPanel CSS to use the component directly

b30ddb1

Delete Sass files

533a4c2

[EuiDroppable] Tests

c940dce

- convert all tests to RTL - add shouldRenderCustomStyles tests - switch modifier class to non BEM to indicate state vs styles

[EuiDraggable] Tests

dd887ab

- add `shouldRenderCustomStyles` test - DRY out test context wrapper logic - fixed `...requiredProps` logic - switch from screen usage to destructured APIs

[EuiDragDropContext] Convert from Enzyme to RTL

0c85e13

- remove unnecessary/unused jest mock

Update downstream snapshots

e6e8ce2

+ fix `false` className by using `classNames()` util

Fix failing datagrid test

1793dd4

- extra selector specifier needed due to new Emotion wrapper around drag/drop components

changelog

283792a

cee-chen force-pushed the emotion/drag-drop branch from 5342a6d to 283792a Compare September 15, 2023 00:51

cee-chen requested review from a team and breehall September 15, 2023 00:52

cee-chen marked this pull request as ready for review September 15, 2023 00:52

breehall approved these changes Sep 18, 2023

View reviewed changes

cee-chen merged commit d1b54f6 into elastic:main Sep 18, 2023
2 checks passed

cee-chen deleted the emotion/drag-drop branch September 18, 2023 16:23

This was referenced Sep 18, 2023

[Emotion] EuiDragAndDrop #6396

Closed

Upgrade EUI to v88.5.0 elastic/kibana#166868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert `EuiDroppable` & `EuiDraggable` #7187

[Emotion] Convert `EuiDroppable` & `EuiDraggable` #7187

cee-chen commented Sep 14, 2023 •

edited by breehall

Loading

cee-chen commented Sep 15, 2023 •

edited

Loading

kibanamachine commented Sep 15, 2023

elasticmachine commented Sep 15, 2023

breehall left a comment

breehall Sep 18, 2023

breehall Sep 18, 2023

cee-chen Sep 18, 2023 •

edited

Loading

[Emotion] Convert EuiDroppable & EuiDraggable #7187

[Emotion] Convert EuiDroppable & EuiDraggable #7187

Conversation

cee-chen commented Sep 14, 2023 • edited by breehall Loading

Summary

QA

General checklist

Emotion checklist

cee-chen commented Sep 15, 2023 • edited Loading

kibanamachine commented Sep 15, 2023

elasticmachine commented Sep 15, 2023

💚 Build Succeeded

History

breehall left a comment

Choose a reason for hiding this comment

breehall Sep 18, 2023

Choose a reason for hiding this comment

breehall Sep 18, 2023

Choose a reason for hiding this comment

cee-chen Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

[Emotion] Convert `EuiDroppable` & `EuiDraggable` #7187

[Emotion] Convert `EuiDroppable` & `EuiDraggable` #7187

cee-chen commented Sep 14, 2023 •

edited by breehall

Loading

cee-chen commented Sep 15, 2023 •

edited

Loading

cee-chen Sep 18, 2023 •

edited

Loading