[Emotion] Convert EuiSideNav by cee-chen · Pull Request #7521 · elastic/eui

cee-chen · 2024-02-09T19:42:07Z

Summary

closes #6412

This PR is on the larger side especially in terms of cleanup, so I strongly recommend following along by commit.

Breaking-ish changes

EuiSideNav now correctly flags a Typescript error when the items prop is missing (wasn't previously for whatever reason)
Removed all EuiSideNav Sass variables (no usages in Kibana)
Removed the @euiSideNavEmbellish Sass mixin (⚠️ 2 usages in Kibana, should be replaced by a JSX prop instead)
Removed the .euiSideNavItemButton__labelContainer CSS selector which didn't appear to be actually applying to anything in either EUI or Kibana?
Fixed the style prop on EuiSideNavItem not applying to the same location as css and className (not a public export)
Removed the following modifier classes that have no Kibana usages:
- .euiSideNav__contentMobile-xs through .euiSideNav__contentMobile-xl (including m, l, etc)
- .euiSideNavItemButton--isClickable
- .euiSideNavItem--rootIcon
- Renamed .euiSideNavItem--hasChildItems to .euiSideNavItem-hasChildItems (single hyphen vs double to indicate state)
Mobile changes
- Changed the padding/height of the mobile toggle nav button to inherit from the wrapping heading element font size
- Added an aria-label fallback for mobile buttons with no heading or mobileTitle props
- The mobile nav now transitions/animates back up on close instead of closing immediately

Tech debt changes

Created a new EuiSideNavHeading sub-component
Cleaned up and flattened a ton of CSS/selectors
Removed let node type JS in favor of inline conditional JSX

QA

Confirm staging looks the same as production
Test staging storybook and confirm that UI looks as expected/not obviously broken, including props toggles
- NOTE: in comparison to production, some things are intentionally different:
- Examples have been changed to showcase a greater variety of styles
- The spacing/padding around the mobile toggle button is different (prod storybook was wrong here. prod EUI docs remain the same)
- The Render Item demo has different text sizing on the root level items. This is expected due to changed CSS selectors and is probably how it should look anyway

General checklist

Updated Storybook
Emotion 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 - N/A
Code quality checklist - N/A
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

- [perf] removes the need for `let headingNode` - cleans up and organizes code

+ remove extra unnecessary `breakpoints` const

- the theme HOC is now causing typescript to correctly flag missing `items` props, no idea why it wasn't doing this previously

+ opinionatedly tweak button padding/height - it's already regressed from pre-Emotion anyway due to the EuiButton conversion + tweak animation to feel a little smoother on EUI docs site + fix `isOpenOnMobile` tests that don't assert anything meaningful because jsdom isn't actually in the mobile breakpoint

- start with button component - reorder imports + convert function to arrow syntax

+ remove `--isClickable` modifier (use CSS selectors instead)

- Remove unnecessary `.euiSideNavItemButton__content` wrapper - just put it on the parent `<button>` - Remove unused `.euiSideNavItemButton__labelContainer` CSS?? doesn't appear to be used anywhere - replace margin with flex gap - remove unnecessary `const`s and just write inline JSX

+ fold in Amsterdam override

+ remove unnecessary CSS and replace let with inline conditional JSX

…o EuiSideBar

- remove now-unnecessary `cloneElement` - isn't needed anymore since we use flex `gap` instead of applying a margin to the icon - move let caret to inline conditional JSX - DRY out various depth-related conditions to more readable variables - memoize callback

doesn't need a changelog as this is not a publicly exported component

- actually needed for custom `renderItem` usages that might not apply the passed button styles down + tweak stories to show an example of correctly passed styles

+ fix nested caret alignment

kibanamachine · 2024-02-09T22:40:55Z

Preview staging links for this PR:

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

elasticmachine · 2024-02-09T22:40:58Z

💚 Build Succeeded

Buildkite Build
Commit: bfbe5ee

History

💚 Build #1391 succeeded 7c47e52

tkajtoch

Code changes look great! I tested desktop and mobile versions and haven't found any regressions. 🚢

`v93.1.1`⏩ `v93.2.0` --- - Updated `EuiPageSidebar` and `EuiPageTemplate.Sidebar` with a new `hasEmbellish` prop (defaults to false) ([#7521](elastic/eui#7521)) - Added `diff` glyph to `EuiIcon` ([#7520](elastic/eui#7520)) - Added `newChat` glyph to `EuiIcon` ([#7524](elastic/eui#7524)) **Bug fixes** - Fixed `EuiSideNav` not correctly typing the `items` prop as required ([#7521](elastic/eui#7521)) - Fixed the `CSS is not defined` bug in `EuiPageTemplate` when rendering in some SSR environments, particularly Next.js v13 and up ([#7525](elastic/eui#7525)) - Fixed `EuiDataGrid` component to clean up timer from side effect on unmount ([#7534](elastic/eui#7534)) **Accessibility** - Fixed `EuiSideNav` to render a fallback aria-label on mobile toggles if no heading or mobile title exists ([#7521](elastic/eui#7521)) **CSS-in-JS conversions** - Converted `EuiSideNav` to Emotion; Removed the following Sass variables: ([#7521](elastic/eui#7521)) - `$euiSideNavEmphasizedBackgroundColor` - `$euiSideNavRootTextcolor` - `$euiSideNavBranchTextcolor` - `$euiSideNavSelectedTextcolor` - `$euiSideNavDisabledTextcolor` - Removed the `euiSideNavEmbellish` Sass mixin. Use the new `EuiPageSidebar` `hasEmbellish` prop instead ([#7521](elastic/eui#7521)) - Added a new memoization/performance optimization utility for CSS-in-JS styles ([#7529](elastic/eui#7529))

`v93.1.1`⏩ `v93.2.0` --- - Updated `EuiPageSidebar` and `EuiPageTemplate.Sidebar` with a new `hasEmbellish` prop (defaults to false) ([elastic#7521](elastic/eui#7521)) - Added `diff` glyph to `EuiIcon` ([elastic#7520](elastic/eui#7520)) - Added `newChat` glyph to `EuiIcon` ([elastic#7524](elastic/eui#7524)) **Bug fixes** - Fixed `EuiSideNav` not correctly typing the `items` prop as required ([elastic#7521](elastic/eui#7521)) - Fixed the `CSS is not defined` bug in `EuiPageTemplate` when rendering in some SSR environments, particularly Next.js v13 and up ([elastic#7525](elastic/eui#7525)) - Fixed `EuiDataGrid` component to clean up timer from side effect on unmount ([elastic#7534](elastic/eui#7534)) **Accessibility** - Fixed `EuiSideNav` to render a fallback aria-label on mobile toggles if no heading or mobile title exists ([elastic#7521](elastic/eui#7521)) **CSS-in-JS conversions** - Converted `EuiSideNav` to Emotion; Removed the following Sass variables: ([elastic#7521](elastic/eui#7521)) - `$euiSideNavEmphasizedBackgroundColor` - `$euiSideNavRootTextcolor` - `$euiSideNavBranchTextcolor` - `$euiSideNavSelectedTextcolor` - `$euiSideNavDisabledTextcolor` - Removed the `euiSideNavEmbellish` Sass mixin. Use the new `EuiPageSidebar` `hasEmbellish` prop instead ([elastic#7521](elastic/eui#7521)) - Added a new memoization/performance optimization utility for CSS-in-JS styles ([elastic#7529](elastic/eui#7529))

Split out EuiSideNavHeading sub-component

bf65dda

- [perf] removes the need for `let headingNode` - cleans up and organizes code

cee-chen added the emotion label Feb 9, 2024

cee-chen added 8 commits February 9, 2024 11:55

Fix screenReaderOnly mobile behavior + add aria-label fallback

46f685e

Remove unnecssary let mobileNode in favor of inline conditional JSX

d089993

+ remove extra unnecessary `breakpoints` const

Wrap EuiSideNav in theme HOC + subsequent type fixes

21da6ef

- the theme HOC is now causing typescript to correctly flag missing `items` props, no idea why it wasn't doing this previously

Set up EuiSideNavItem Emotion conversion

031c60c

- start with button component - reorder imports + convert function to arrow syntax

Convert item button modifier styles

9762a98

+ remove `--isClickable` modifier (use CSS selectors instead)

Convert + flatten remaining euiSideNavItemButton styles

9644a6f

+ fold in Amsterdam override

cee-chen force-pushed the emotion/side_nav branch 3 times, most recently from 661b18d to 80678f4 Compare February 9, 2024 22:05

cee-chen added 10 commits February 9, 2024 14:12

Convert layout styles for euiSideNavItem

bb055f3

Convert euiSideNavItem__items

84e9d50

+ remove unnecessary CSS and replace let with inline conditional JSX

Convert remaining emphasized styles

a4f453e

Remove euiSideNavEmbellish Sass mixin for new hasEmbellish prop t…

2c385a9

…o EuiSideBar

Delete all Sass variables and files

612ca41

Misc component cleanup

cd8b9c9

- remove now-unnecessary `cloneElement` - isn't needed anymore since we use flex `gap` instead of applying a margin to the icon - move let caret to inline conditional JSX - DRY out various depth-related conditions to more readable variables - memoize callback

Update snapshots

b4e1697

Make all style-related props pass correctly to EuiSideNavItem

fa18449

doesn't need a changelog as this is not a publicly exported component

Restore .euiSideNavItemButton__content wrapper

206f1b4

- actually needed for custom `renderItem` usages that might not apply the passed button styles down + tweak stories to show an example of correctly passed styles

[storybook] Improve default story to show off more styles/states

bfbe5ee

+ fix nested caret alignment

cee-chen force-pushed the emotion/side_nav branch from 80678f4 to bfbe5ee Compare February 9, 2024 22:12

cee-chen marked this pull request as ready for review February 9, 2024 22:27

cee-chen requested a review from a team as a code owner February 9, 2024 22:27

tkajtoch self-requested a review February 13, 2024 17:04

tkajtoch approved these changes Feb 14, 2024

View reviewed changes

cee-chen merged commit a428963 into elastic:main Feb 14, 2024

cee-chen deleted the emotion/side_nav branch February 14, 2024 16:49

cee-chen mentioned this pull request Feb 20, 2024

Upgrade EUI to v93.2.0 elastic/kibana#177361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert EuiSideNav#7521

[Emotion] Convert EuiSideNav#7521
cee-chen merged 19 commits intoelastic:mainfrom
cee-chen:emotion/side_nav

cee-chen commented Feb 9, 2024 •

edited by tkajtoch

Loading

Uh oh!

kibanamachine commented Feb 9, 2024

Uh oh!

elasticmachine commented Feb 9, 2024

Uh oh!

tkajtoch left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

cee-chen commented Feb 9, 2024 • edited by tkajtoch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking-ish changes

Tech debt changes

QA

General checklist

Uh oh!

kibanamachine commented Feb 9, 2024

Uh oh!

elasticmachine commented Feb 9, 2024

💚 Build Succeeded

History

Uh oh!

tkajtoch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

cee-chen commented Feb 9, 2024 •

edited by tkajtoch

Loading