[Emotion] Convert `EuiResizableButton` by cee-chen · Pull Request #7081 · elastic/eui

cee-chen · 2023-08-14T22:33:57Z

Summary

#6408

This PR converts EuiResizableButton to Emotion
- This PR does not convert EuiResizableCollapseButton. That conversion + this one was getting fairly gnarly, so I opted to leave it separate to make review easier.
This PR performs several incidental cleanup items on EuiResizableButton, including adding unit tests that were previously missing, and gets component code coverage up to 100%
This PR performs a large CSS refactor (78e57b8) that converts from absolute positioning & transform CSS to using flex CSS instead. This has the major benefit of:
1. fixing an existing Safari visual bug on production (go to https://eui.elastic.co/v87.0.0/#/layout/resizable-container on Safari and hover over any drag handle), and
2. also makes the component fully work with logical properties, as transform: translateX/Y currently does not respect direction or writing-mode and flex does.

QA

Compare staging against production and confirm the grab handle (not the collapsible button) looks and behaves as before (except on Safari, which has broken CSS transitions on production)
Go to http://localhost:8030/#/layout/resizable-container#collapsible-panels-with-external-control
Inspect the first resizable button / drag handle in the DOM
Click the "Toggle panel 1" button in the demo
In the DOM, confirm the button is hidden with display: none
Confirm that the now-hidden button cannot be interacted with in any way, e.g. via mouseover or keyboard tab or screen reader navigation

General checklist

Checked in both light and dark modes
- [ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

Emotion checklist

Kibana usage

Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes
- ~~[ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into~~
- ⚠️ Only one modifier usage exists, and it's one I added earlier to override Sass behavior. It should be completely removable

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

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

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)
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 - One instance
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

~~[ ] Documentation pass:~~
Reduced specificity where possible (usually by reducing nesting and class name chaining)
~~[ ] 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~~

- not sure why tsc wasn't picking up on this 🤷

- mostly for custom styles testing, but we might as well write some missing unit tests while we're here

- for code coverage, because I'm incredibly anal + misc code cleanup - optional chaining syntax, remove unneccessary fn declarations

in favor of JS logic + HTML attribute - `hidden` is equivalent to `display: none` and is not overrideable via CSS and thus does not require an important - this cleanup allows us to remove unnecessary `:disabled` selectors in favor of simpler ones - if the button is disabled, we can assume it's never visible or interactable

+ remove $euiResizableButtonTransitionSpeed

+ Remove all now-unnecessary modifier clases + Remove `$euiResizableButtonSize` Sass var

kibanamachine · 2023-08-14T23:04:26Z

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

- vs absolute positioning/transforms Doing this has two major benefits: - The hover/focus animations were broken on Safari in production, and this fixes that - CSS transform do not respect logical properties and flex does, so this automatically makes the component fully direction/writing-mode compatible for less code :)

kibanamachine · 2023-08-14T23:50:17Z

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

The `hidden` attribute sadly gets overridden by the new `display: flex`, so we need to restore the `:disabled { display: none }` selector

cee-chen · 2023-08-15T01:49:30Z

As a heads up, I'm planning on following up this PR not just with another EuiResizableCollapse button conversion, but also with the following backlog items for EuiResizableButton:

Export the EuiResizable button #6100 (this will add a Storybook for EuiResizableButton and make it generally easier to QA later)
[EuiResizableContainer] Configurable position for EuiResizableButton indicator #5787

kibanamachine · 2023-08-15T02:04:31Z

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

elasticmachine · 2023-08-15T02:23:11Z

💚 Build Succeeded

Buildkite Build
Commit: 998cd84

History

💚 Build #343 succeeded 78e57b8
💔 Build #341 failed b3cffa1

breehall

Hey Cee, I just have a question here about the decision to hide the resizable button when not in use. Other than that, this conversion looks good. Here was my review process:

Reviewed the Sass conversion to ensure it's in parody with production. Loved the CSS refactor with flex
Ensured the resizable button is hidden in the DOM when a panel is toggled off in the docs
Observed improved transitions in Safari due to the CSS refactor

breehall · 2023-08-15T18:34:29Z

src/components/resizable_container/resizable_button.test.tsx

+    it('renders as disabled if the disabled prop is passed', () => {
+      const { container } = render(<EuiResizableButton disabled />, {
+        wrapper,
+      });
+
+      expect(container.firstChild).toBeDisabled();
+    });


Can I ask why we choose to render the button as disabled with the style display:none instead of removing it from the DOM when it's not in use by a resizable panel?

I'm honestly not sure - I think it was part of the original component design and I can't say exactly why those devs chose to go with display: none vs simply not rendering anything.

I did play around with returning null if disabled, but in the end I decided against it - the potential for downstream impacts felt too large for what was supposed to be "just" an Emotion conversion. Also, the 'collapsed' content uses a similar paradigm (display: none on all children) so I felt like it made sense to keep that for now so they stay in-line with one another.

the potential for downstream impacts felt too large for what was supposed to be "just" an Emotion conversion

Totally makes sense, I was just curious!

breehall · 2023-08-15T19:15:58Z

src/components/resizable_container/resizable_button.styles.ts

+      flex-shrink: 0;
+      display: flex;
+      align-items: center;
+      justify-content: center;
+      gap: ${mathWithUnits(grabHandleHeight, (x) => x * 2)};
+
+      /* 1 */
+      &:hover,
+      &:focus {
+        gap: 0;
+        justify-content: center;
+      }
+
+      ${euiCanAnimate} {
+        transition: gap ${transition}, justify-content ${transition};
+      }


[Attaching to closes code snippet] This is a nice update. Flex is easier to visualize and understand when looking at the code (at least for me)!

Agreed, it makes the code way cleaner!

breehall · 2023-08-15T19:26:36Z

src/components/resizable_container/resizable_button.tsx

+  const {
+    registry: { resizers } = { resizers: {} } as EuiResizableContainerRegistry,
+  } = useEuiResizableContainerContext();


Odd that VSCode would flag this and not our linter

I've had 1:1 disparities between vscode types and CLI tsc in the past, so it's not terribly uncommon, but it is mildly annoying. I think vscode is a bit more opinionated than the CLI is. 🤷

breehall

Thank you for taking a look at my question. Everything has been answered and I have no further questions.

cee-chen · 2023-08-15T19:40:28Z

Thanks Bree!

`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 7 commits August 14, 2023 10:55

Set up Emotion styles file

9c80d8a

[misc cleanup] Fix vscode type complaint

ede8fb0

- not sure why tsc wasn't picking up on this 🤷

[tech debt] Set up missing tests

a200128

- mostly for custom styles testing, but we might as well write some missing unit tests while we're here

[misc cleanup] Write more EuiResizableButton tests

89a22b0

- for code coverage, because I'm incredibly anal + misc code cleanup - optional chaining syntax, remove unneccessary fn declarations

Convert more base styles to Emotion

a7eb4b5

+ remove $euiResizableButtonTransitionSpeed

Convert remaining horizontal/vertical styles to Emotion

c473464

+ Remove all now-unnecessary modifier clases + Remove `$euiResizableButtonSize` Sass var

cee-chen added the emotion label Aug 14, 2023

cee-chen added 4 commits August 14, 2023 16:27

Remove Sass files

863e237

changelog

60d6a90

[Emotion checklist] Add missing motion query for transitions

86ea8e5

cee-chen force-pushed the emotion/resizable-button branch from 14039f7 to 78e57b8 Compare August 14, 2023 23:27

Fix disabled hiding

998cd84

The `hidden` attribute sadly gets overridden by the new `display: flex`, so we need to restore the `:disabled { display: none }` selector

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

cee-chen requested review from a team and breehall August 15, 2023 01:42

breehall reviewed Aug 15, 2023

View reviewed changes

breehall approved these changes Aug 15, 2023

View reviewed changes

cee-chen merged commit 6a30eba into elastic:main Aug 15, 2023

cee-chen deleted the emotion/resizable-button branch August 15, 2023 19:40

cee-chen mentioned this pull request Aug 17, 2023

[Emotion] EuiResizableContainer #6408

Closed

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

Conversation

cee-chen commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Emotion checklist

Uh oh!

kibanamachine commented Aug 14, 2023

Uh oh!

kibanamachine commented Aug 14, 2023

Uh oh!

cee-chen commented Aug 15, 2023

Uh oh!

kibanamachine commented Aug 15, 2023

Uh oh!

elasticmachine commented Aug 15, 2023

💚 Build Succeeded

History

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

breehall Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Aug 14, 2023 •

edited

Loading