Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Emotion] Convert EuiSideNav #7521

Merged
merged 19 commits into from
Feb 14, 2024
Merged

[Emotion] Convert EuiSideNav #7521

merged 19 commits into from
Feb 14, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Feb 9, 2024 •
edited by tkajtoch
Loading

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

@cee-chen
Split out EuiSideNavHeading sub-component
bf65dda 
- [perf] removes the need for `let headingNode`

- cleans up and organizes code
@cee-chen
Fix screenReaderOnly mobile behavior + add aria-label fallback
46f685e
@cee-chen
Remove unnecssary let mobileNode in favor of inline conditional JSX
d089993 
+ remove extra unnecessary `breakpoints` const
@cee-chen
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
@cee-chen
Convert mobile styles to Emotion
fae4abc 
+ 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
@cee-chen
Set up EuiSideNavItem Emotion conversion
031c60c 
- start with button component

- reorder imports + convert function to arrow syntax
@cee-chen
Convert item button modifier styles
9762a98 
+ remove `--isClickable` modifier (use CSS selectors instead)
@cee-chen
Convert item button child styles + lots of cleanup
0f15f89 
- 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
@cee-chen
Convert + flatten remaining euiSideNavItemButton styles
9644a6f 
+ fold in Amsterdam override
@cee-chen 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
Convert layout styles for euiSideNavItem
bb055f3
@cee-chen
Convert euiSideNavItem__items
84e9d50 
+ remove unnecessary CSS and replace let with inline conditional JSX
@cee-chen
Convert remaining emphasized styles
a4f453e
@cee-chen
Remove euiSideNavEmbellish Sass mixin for new hasEmbellish prop t…
2c385a9 
…o EuiSideBar
@cee-chen
Delete all Sass variables and files
612ca41
@cee-chen
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
@cee-chen
Update snapshots
b4e1697
@cee-chen
Make all style-related props pass correctly to EuiSideNavItem
fa18449 
doesn't need a changelog as this is not a publicly exported component
@cee-chen
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
@cee-chen
[storybook] Improve default story to show off more styles/states
bfbe5ee 
+ fix nested caret alignment
@cee-chen cee-chen marked this pull request as ready for review February 9, 2024 22:27
@cee-chen cee-chen requested a review from a team as a code owner February 9, 2024 22:27
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@tkajtoch tkajtoch self-requested a review February 13, 2024 17:04
tkajtoch
tkajtoch approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cee-chen cee-chen merged commit a428963 into elastic:main Feb 14, 2024
8 checks passed
@cee-chen cee-chen deleted the emotion/side_nav branch February 14, 2024 16:49
@cee-chen cee-chen mentioned this pull request Feb 20, 2024
Merged
jbudz pushed a commit to elastic/kibana that referenced this pull request Feb 23, 2024
@cee-chen
Upgrade EUI to v93.2.0 (#177361)
7481923 
`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))
semd pushed a commit to semd/kibana that referenced this pull request Mar 4, 2024
@cee-chen @semd
Upgrade EUI to v93.2.0 (elastic#177361)
192c681 
`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))
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@cee-chen @fkanout
Upgrade EUI to v93.2.0 (elastic#177361)
b761b46 
`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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkajtoch tkajtoch tkajtoch approved these changes

Assignees
No one assigned
Labels
emotion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Emotion] EuiSideNav
4 participants
@cee-chen @kibanamachine @elasticmachine @tkajtoch