[Emotion] Convert `EuiResizableCollapseButton` #7091

cee-chen · 2023-08-17T00:35:58Z

Summary

closes #6408 (final component)

Ohh boy, this is a fun one. There was a lot of very branch-y Sass to untangle that I ended up basically rewriting in ab23346, so buckle your seatbelts and as always, follow along by commit!

QA

yarn compile-scss && yarn storybook
Go to http://localhost:6006/?path=/story/euiresizablecollapsebutton--production-usage
Confirm that the direction and position behave as follows:
- When the direction is horizontal:
  - top displays the collapse button at the top when both collapsed and not collapsed
  - bottom displays the collapse button at the bottom when both collapsed and not collapsed
  - middle, left, and right display the collapse button vertically centered for all positions
- When the direction is vertical:
  - left displays the collapse button on left when both collapsed and not collapsed
  - right displays the collapse button at the right when both collapsed and not collapsed
  - middle, top, and bottom display the collapse button horizontally centered for all positions
  - Change the position back to left and change the writing mode in the toolbar from LTR to RTL. Confirm that the button correctly displays on the right instead of the left
[Optional] Go to http://localhost:6006/?path=/story/euiresizablecollapsebutton--playground and play with all props controls to get a sense of the component

General checklist

Checked in both light and dark modes
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs (using @default if default values are missing) and storybook toggles
A changelog entry exists and is marked appropriately

Emotion checklist

Kibana usage

Search Kibana's codebase for euiResizableToggleButton- (case sensitive) to check for usage of modifier classes
- ✅ No usages whatsoever exist

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~~

Unit tests

No tests added/written as this component is internal-only and did not previously have any tests

~[ ] shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
~~[ ] Removed any mount()ed snapshots in favor of render() or a more specific assertion~~

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~~
~~[ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~
~~[ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~
~~[ ] Removed component from src/components/index.scss~~ - Not doable yet
~~[ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)~~

CSS tech debt

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

DOM Cleanup

Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)
SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Kibana due diligence

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:
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) - No usages

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~~ - Will be opening up a follow-up PR for this
~~[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about~~

+ move `...rest` order - `className` does not need to come after since it's already extracted from `rest`

+ clean up unnecessary CSS unsets via ternary logic + remove comment TODO - this component uses it, so it's not just panels

- Reuse `EuiScreenReaderOnly`'s focus styles so we're not repeating selectors - NOTE: We can't use `<EuiScreenReaderOnly showOnFocus>` directly unfortunately, because the conditional logic causes the component to completely unmount and focus to be stranded + remove now-unnecessary `-isVisible` class modifier

+ set up the branching JS logic for it - it's going to be too complex for ternaries

- there isn't an easy way to show the migration path I took because I basically just rewrote it from scratch to avoid nesting and having to repeatedly override/unset CSS - I opted to switch from transforms to negative margins so we can avoid having to override EuiButtonIcon's default transform CSS

- switch from if branches/pushes to arrays and ternaries

`externalPosition` is always defined in EUI usage, but setting a default is nicer for storybook and simplifying logic `direction` is also always defined in usage as well and has a default here + remove unnecessary logic/icon that never gets used

- now that we don't need to set transforms, we can let the default button transition/transform do its thing

- remove modifiers - no usages in Kibana - fix className to match component name

cee-chen · 2023-08-17T01:01:56Z

src/components/resizable_container/resizable_collapse_button.styles.ts

+  const { euiTheme } = euiThemeContext;
+
+  const buttonSize = euiTheme.size.l;
+  const centeringOffset = mathWithUnits(buttonSize, (x) => x / -2); // Use negative margins instead of transforms to avoid having to override EuiButtonIcon's CSS


Also worth noting that switching to negative margins (instead of transform/translate) has the advantage of making the the component fully obey logical properties, and work correctly in various direction/writing modes.

kibanamachine · 2023-08-17T01:02:54Z

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

cee-chen · 2023-08-17T01:08:30Z

src/components/resizable_container/resizable_collapse_button.styles.ts

+        // `left/right` aren't valid positions for the horizontal direction,
+        // so we're using getters to fall back to the `middle` CSS
+        get left() {
+          return this.middle;
+        },
+        get right() {
+          return this.middle;
+        },


In the past where we've needed to repeat fairly complex modifiers in Emotion objects, I've previously done something like:

const styles = ({ euiTheme }) => { const styles = { hello: css` // ... lots of css `, world: css``, }; styles.world = hello; return styles; };

i.e.:

eui/src/components/steps/step.styles.ts

Line 102 in 8d21215

s: css``, // s is the same as m, so we'll programmatically duplicate it below

eui/src/components/steps/step.styles.ts

Line 119 in 8d21215

styles.s = styles.m;

I really like this getter option instead - it may feel a little overengineered, but it saves having to define a object name and essentially empty key!

elasticmachine · 2023-08-17T01:17:25Z

💚 Build Succeeded

Buildkite Build
Commit: d97b762

breehall

It was excited to see how you refactored these styles as I walked through the commits. You did a great job here making this very complex Sass seem like a cakewalk with Emotion. Because your commits were broken up so nicely, it was easy for me to understand the transformation and confirm that the styles work as you describe in the PR description. I don't have any technical notes or recommendations to add to this one.

Thanks to Storybook, confirming these changes was a breeze. I can only imagine it would have been a nightmare with the playground we currently have in the docs. Great job 👍🏾

breehall · 2023-08-17T16:56:38Z

src/components/resizable_container/resizable_collapse_button.tsx

+  const collapsedStyles = [
+    styles.collapsed.collapsed,
+    styles.collapsed[direction],
+    styles.collapsed[`${direction}Positions`][internalPosition],
+  ];
+  const collapsibleStyles = [
+    styles.collapsible.collapsible,
+    styles.collapsible[direction][externalPosition],
+    styles.collapsible[direction][internalPosition],
+  ];
+  const cssStyles = [
+    styles.euiResizableCollapseButton,
+    showOnFocus && screenReaderOnlyStyles,
+    ...(isCollapsed ? collapsedStyles : collapsibleStyles),
+  ];


Excellent work here!

Thanks Bree! I'd be super curious to hear if you like reading or writing this syntax more than the if/pushes that I ended up refactoring away from!

Personally, I like this syntax more! The styles for this component are quite intertwined and as such, the switch statements were a little lengthier. If I was adding styles or debugging this component, it would be a lot easier for me to look at these style arrays and reference their objects in Emotion. It would take less time for me to understand and jump into action where needed.
Just to be clear, I don't think it would take a lot of time to understand the switch statements, but because this is very human readable I don't have to spend the couple of seconds to trace through the styling logic.

Fantastic, that's all great to hear - thanks!

`87.1.0` ➡️ `87.2.0` ## [`87.2.0`](https://github.com/elastic/eui/tree/v87.2.0) - `EuiResizableButton` is now available as a generic top-level export ([#7087](elastic/eui#7087)) - Added new `alignIndicator` prop to `EuiResizableButton`. Defaults to `center`, and can now additionally be configured to `start` and `end` ([#7087](elastic/eui#7087)) - Updated `useGeneratedHtmlId` hook to use `React.useId` as the source of unique identifiers when available ([#7095](elastic/eui#7095)) **CSS-in-JS conversions** - Converted `EuiResizableButton` to Emotion; Removed `$euiResizableButtonTransitionSpeed` and `$euiResizableButtonSize` ([#7081](elastic/eui#7081)) - Converted `EuiResizableCollapseButton` to Emotion ([#7091](elastic/eui#7091)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co>

cee-chen added 13 commits August 16, 2023 14:00

Set up Emotion file

eb190d5

+ move `...rest` order - `className` does not need to come after since it's already extracted from `rest`

Convert more styles to Emotion

4848f8e

+ clean up unnecessary CSS unsets via ternary logic + remove comment TODO - this component uses it, so it's not just panels

[setup] split up collapsed vs non-collapsed styles

5e97351

+ set up the branching JS logic for it - it's going to be too complex for ternaries

[refactor] replace branches/switches with getters

1d31e2f

[refactor] styles syntax

2909d48

- switch from if branches/pushes to arrays and ternaries

[opinionated change] Restore transform transition

6c17a2b

- now that we don't need to set transforms, we can let the default button transition/transform do its thing

Clean up classNames

32e87f2

- remove modifiers - no usages in Kibana - fix className to match component name

[misc cleanup] remove unused comment

774427e

Delete Sass files

876090d

Add stories

0a80992

cee-chen mentioned this pull request Aug 17, 2023

[Emotion] EuiResizableContainer #6408

Closed

changelog

d97b762

cee-chen added the emotion label Aug 17, 2023

cee-chen requested review from a team and breehall August 17, 2023 00:59

cee-chen marked this pull request as ready for review August 17, 2023 01:00

cee-chen commented Aug 17, 2023

View reviewed changes

breehall approved these changes Aug 17, 2023

View reviewed changes

cee-chen merged commit 7ff8339 into elastic:main Aug 17, 2023

cee-chen deleted the emotion/resizable-collapse-button branch August 17, 2023 22:11

cee-chen mentioned this pull request Aug 20, 2023

[Emotion] Document usage of getters in Emotion style objects #7104

Merged

2 tasks

breehall mentioned this pull request Aug 22, 2023

Upgrade EUI to 87.2.0 elastic/kibana#164385

Merged

1Copenut mentioned this pull request Aug 28, 2023

Upgrade EUI to v88.1.0 elastic/kibana#165047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert `EuiResizableCollapseButton` #7091

[Emotion] Convert `EuiResizableCollapseButton` #7091

cee-chen commented Aug 17, 2023 •

edited by breehall

Loading

cee-chen Aug 17, 2023 •

edited

Loading

kibanamachine commented Aug 17, 2023

cee-chen Aug 17, 2023

elasticmachine commented Aug 17, 2023

breehall left a comment

breehall Aug 17, 2023

cee-chen Aug 17, 2023 •

edited

Loading

breehall Aug 17, 2023 •

edited

Loading

cee-chen Aug 17, 2023

[Emotion] Convert EuiResizableCollapseButton #7091

[Emotion] Convert EuiResizableCollapseButton #7091

Conversation

cee-chen commented Aug 17, 2023 • edited by breehall Loading

Summary

QA

General checklist

Emotion checklist

cee-chen Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Aug 17, 2023

cee-chen Aug 17, 2023

Choose a reason for hiding this comment

elasticmachine commented Aug 17, 2023

💚 Build Succeeded

breehall left a comment

Choose a reason for hiding this comment

breehall Aug 17, 2023

Choose a reason for hiding this comment

cee-chen Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

breehall Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 17, 2023

Choose a reason for hiding this comment

[Emotion] Convert `EuiResizableCollapseButton` #7091

[Emotion] Convert `EuiResizableCollapseButton` #7091

cee-chen commented Aug 17, 2023 •

edited by breehall

Loading

cee-chen Aug 17, 2023 •

edited

Loading

cee-chen Aug 17, 2023 •

edited

Loading

breehall Aug 17, 2023 •

edited

Loading