[EuiSideNav] Migrate from class to function component#9831
Conversation
Convert EuiSideNav from a withEuiTheme-wrapped class to a generic function component: useEuiTheme replaces the HOC, destructured defaults replace defaultProps, and the instance methods become local closures. Behavior and the public props API are unchanged (all snapshots pass as-is). generateId is memoized so the generated DOM ids stay stable across renders, matching the previous class-field behavior.
|
💚 CLA has been signed |
|
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
github-actions
Bot
added
the
community contribution
There was a problem hiding this comment.
Pull request overview
Migrates EuiSideNav from a withEuiTheme-wrapped class component to a generic function component using hooks, while preserving existing rendering behavior and snapshot output.
Changes:
- Replaced
withEuiTheme/WithEuiThemePropswith theuseEuiTheme()hook. - Converted class instance methods (
isItemOpen,renderTree) into local closures inside the function component. - Replaced
static defaultPropswith default values in props destructuring and memoized thehtmlIdGeneratorto keep IDs stable across re-renders.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @youdie006 👋🏻 before we review your contribution, could you sign CLA? 🙏🏻 |
|
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
|
Thanks @weronikaolejniczak! I've signed the Elastic CLA now - the CLA check is green. Ready for review whenever you have a chance. |
weronikaolejniczak
added
the
skip-changelog
There was a problem hiding this comment.
I only found one nit. The behavior is preserved, changes minimal, CI passed (including unit, Cypress and VRT tests). I added skip-changelog label. All looks great! 🟢 Thank you for contributing, @youdie006!
Feel free to push the commit for that one nit, otherwise we'll take it from there.
EDIT: No need to rebase onto main unless there are conflicts. Otherwise, we will need to keep re-running CI.
Replace the manual htmlIdGenerator/useMemo with EUI's useGeneratedHtmlId
hook for the content and heading IDs, per review nit. Behavior is
preserved: useGeneratedHtmlId({ prefix, suffix, conditionalId }) returns
conditionalId || htmlIdGenerator(prefix)(suffix).
💚 Build Succeeded
History
|
💚 Build Succeeded
History
|
Summary
Migrates
EuiSideNavfrom awithEuiTheme-wrapped class to a generic function component, per #9475 and the class-to-function campaign.Changes
withEuiThemeHOC →useEuiTheme()hook.static defaultProps→ destructured default values (items = [],mobileBreakpoints = ['xs', 's']).isItemOpen,renderTree) → local closures.useMemo(() => htmlIdGenerator('euiSideNav'), [])) so thecontent/headingDOM ids stay stable across renders, matching the previous class-field behavior.<T>); previously the export erasedTto{}viawithEuiTheme<EuiSideNavProps>, soTis now genuinely inferred fromitems— strictly more capable, no breaking change.There's no internal state to migrate:
isOpenOnMobile/toggleOpenOnMobileare controlled props owned by the consumer, so nouseStatewas introduced.Verification
Behavior and the public props API are unchanged — all existing snapshots pass as-is (none regenerated).
yarn build:workspaces,yarn test-unitonside_nav.test.tsx+side_nav_item.test.tsx(28 tests, 22 snapshots),yarn tsc --noEmit(0 errors), and eslint all pass.I followed the precedent of the earlier reference PR for this issue (#9533), which changed only
side_nav.tsxwith no changelog entry, since this is an internal, behavior-identical migration — happy to add a changelog note if you'd prefer one.Closes #9475.
AI disclosure: this PR was drafted with Claude Code (AI-assisted) and human-reviewed. I mirrored the already-migrated
EuiCollapsibleNavpattern and verified the migration is behavior-identical (all snapshots unchanged).