[EuiAccordion] Migrate from class to function component#9558
[EuiAccordion] Migrate from class to function component#9558antonbc wants to merge 3 commits intoelastic:mainfrom
Conversation
|
💚 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? |
|
👋 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? |
There was a problem hiding this comment.
Pull request overview
Migrates EuiAccordion from a class component wrapped in withEuiTheme to a function component that uses hooks (useEuiTheme, useGeneratedHtmlId) while aiming to keep behavior and public API the same.
Changes:
- Replaced class state/handlers with
useStateand an internal toggle handler. - Replaced
withEuiTheme+WithEuiThemePropsusage withuseEuiTheme. - Replaced
htmlIdGenerator()instance property withuseGeneratedHtmlId().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setIsOpenState((prevIsOpen) => { | ||
| const nextState = !prevIsOpen; | ||
| onToggle?.(nextState); | ||
| return nextState; | ||
| }); |
There was a problem hiding this comment.
onToggle is invoked inside the setIsOpenState functional updater. React may call state updater functions more than once (e.g., StrictMode/dev, interrupted renders), which can lead to onToggle firing multiple times for a single user interaction. Consider computing nextState in the click handler using the current isOpenState, calling setIsOpenState(nextState), and then calling onToggle(nextState) outside the updater (or triggering the callback from an effect that only runs for user-initiated toggles).
| setIsOpenState((prevIsOpen) => { | |
| const nextState = !prevIsOpen; | |
| onToggle?.(nextState); | |
| return nextState; | |
| }); | |
| const nextState = !isOpenState; | |
| setIsOpenState(nextState); | |
| onToggle?.(nextState); |
There was a problem hiding this comment.
Let's please address this.
| const euiTheme = useEuiTheme(); | ||
| const classes = classNames( | ||
| 'euiAccordion', | ||
| { 'euiAccordion-isOpen': isOpen }, | ||
| className | ||
| ); | ||
|
|
||
| const styles = euiAccordionStyles(euiTheme); |
There was a problem hiding this comment.
useEuiTheme() returns a theme object (with euiTheme, colorMode, etc.), but the local variable is named euiTheme, which reads like it contains only the euiTheme property. Renaming this variable to theme (or destructuring { euiTheme } and adjusting the style call accordingly) would avoid confusion and reduce the chance of misusing the hook return value later.
| const euiTheme = useEuiTheme(); | |
| const classes = classNames( | |
| 'euiAccordion', | |
| { 'euiAccordion-isOpen': isOpen }, | |
| className | |
| ); | |
| const styles = euiAccordionStyles(euiTheme); | |
| const theme = useEuiTheme(); | |
| const classes = classNames( | |
| 'euiAccordion', | |
| { 'euiAccordion-isOpen': isOpen }, | |
| className | |
| ); | |
| const styles = euiAccordionStyles(theme); |
There was a problem hiding this comment.
We actually use euiThemeContext:
const euiThemeContext = useEuiTheme();
const styles = euiAccordionStyles(euiThemeContext);| className | ||
| ); | ||
|
|
||
| const styles = euiAccordionStyles(euiTheme); |
There was a problem hiding this comment.
This was not part of the task but we could use useEuiMemoizedStyles
💚 Build Succeeded |
💚 Build Succeeded
|
|
|
||
| onToggle = () => { | ||
| const { forceState } = this.props; | ||
| export const EuiAccordionClass: FunctionComponent<EuiAccordionProps> = ({ |
There was a problem hiding this comment.
nit:
We should export it directly as EuiAccordion, not EuiAccordionClass because that's no longer true.
| export const EuiAccordionClass: FunctionComponent<EuiAccordionProps> = ({ | |
| export const EuiAccordion: FunctionComponent<EuiAccordionProps> = ({ |
…d useEuiTheme
Summary
EuiAccordionfrom a class component to a function component.useStateforisOpenwithEuiThemeHOC withuseEuiThemehookAPI Changes
No API changes.
Screenshots
No visual changes
Impact Assessment
Note: Most PRs should be tested in Kibana to help gauge their Impact before merging.
🔴 Breaking changes💅 Visual changes🔧 Hard to integrateImpact level: 🟢 None / 🟢 Low / 🟡 Moderate / 🔴 High
Release Readiness
Documentation:Figma:Migration guide:Adoption planQA instructions for reviewer
Checklist before marking Ready for Review
breaking changelabel (if applicable)Reviewer checklist
useStatewithEuiThemeHOC withuseEuiThemehook if applicable