[EuiFilePicker] Migrate from class to function component - #9879
[EuiFilePicker] Migrate from class to function component#9879mehuljariwala wants to merge 5 commits into
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? |
|
buildkite test this |
There was a problem hiding this comment.
Pull request overview
Migrates EuiFilePicker from a class component to a function component while preserving behavior, markup, and the existing test suite expectations for controlled files display state, clearing, and drag-over styling.
Changes:
- Converted class lifecycle/state/refs to hooks (
useState,useRef,useEffect,useCallback) while keeping thefiles-driven prompt text update semantics aligned with the priorcomponentDidUpdate. - Replaced legacy patterns with hook equivalents (
useFormContext,useEuiMemoizedStyles,useGeneratedHtmlId,useEuiI18n) and removed thewithEuiStylesMemoizerHOC/class export. - Kept prompt text state intentionally independent from
filesduring render to preserve the existing “clear whilefilesis still set” behavior covered by tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
weronikaolejniczak
left a comment
There was a problem hiding this comment.
The refactor works great 💪🏻 I didn't notice any regression compared to prod and VRTs pass.
One improvement we could do: add a "MultipleFiles" story that passes the multiple prop or have the control to toggle this in "Playground" story.
Thanks for contributing, @mehuljariwala 🙌🏻
Co-authored-by: Weronika Olejniczak <32842468+weronikaolejniczak@users.noreply.github.com>
Co-authored-by: Weronika Olejniczak <32842468+weronikaolejniczak@users.noreply.github.com>
|
Hello @weronikaolejniczak, Thank you for taking the time to review this. I have a well-defined plan to migrate all the components one by one, which will allow us to maintain a clean codebase and ensure everything remains well-structured and manageable throughout the migration. |
|
@mehuljariwala please be aware we receive contributions only for issues labeled as "help wanted". |
Yes sounds good |
|
@mehuljariwala there's a syntax error. I would advise to test your changes properly before pushing. Spin up Storybook, documentation website! 😄 |
Yes done thank you @weronikaolejniczak |
| styles.euiFilePicker__prompt, | ||
| disabled && styles.disabled, | ||
| ...(normalFormControl | ||
| ? [compressed ? styles.compressed : styles.uncompressed] |
There was a problem hiding this comment.
@mehuljariwala you've addressed the typo but you haven't actually went and updated the style 🤔
There was a problem hiding this comment.
Oh yeah correct give me one moment i am checking on that as well
💔 Build Failed
Failed CI StepsHistory
|
… style The showDrop suggestion was applied over the useCallback body, leaving a stray closing brace and dependency array behind, which broke parsing and failed every Buildkite build. Simplify hideDrop to match, and finish the typo fix by renaming the icon style key itself from 'compresssed' to 'compressed'.
💔 Build Failed
Failed CI StepsHistory
|
Summary
What: Migrates
EuiFilePickerfrom a class component to a function component.Why: Closes #9495.
How:
class EuiFilePickerClass extends ComponentFunctionComponent<EuiFilePickerProps>withEuiStylesMemoizerHOC +stylesMemoizerpropuseEuiMemoizedStyles(euiFilePickerStyles)static contextType = FormContextuseFormContext()static defaultPropsfileInputinstance field + callback refuseRef<HTMLInputElement>this.state/setStateuseState×2componentDidUpdateuseEffectkeyed onfileshtmlIdGenerator()()useGeneratedHtmlId()<EuiI18n>render-prop wrapperuseEuiI18n()Two decisions worth flagging for reviewers:
The
fileseffect keeps a previous-value ref. A bareuseEffect(..., [files])also runs on mount, which would trigger a redundant second render in the multi-file case, sincegetPromptTextFromFileListreturns a freshEuiI18nelement that can never reference-compare equal. The ref guard makes the effect fire only on an actual prop change, matchingcomponentDidUpdateexactly.promptTextis deliberately not derived during render. The existing testclears file name when clear button is clickedclears the input while thefilesprop is still set, so re-deriving from props on every render would resurrect the filename and break it.The
EuiFilePickerClassexport is removed along with the HOC. It was never re-exported fromindex.ts(onlyEuiFilePickerandEuiFilePickerPropsare public), so this stays internal-only.I also swapped
htmlIdGenerator()()foruseGeneratedHtmlId()while here, which clears one more function-component usage tracked in #7093.One note on the issue description: it lists the class as 262 lines with Lifecycle: None, but it is currently 382 lines and does have a
componentDidUpdate, handled as described above.API Changes
None — this is an internal refactor.
EuiFilePickerScreenshots
No visual changes — rendered markup is identical, and the existing snapshot passes unmodified.
Impact Assessment
Impact level: 🟢 None
Release Readiness
Documentation— no doc changes; behavior and API are unchangedFigma— no design changesMigration guide— no breaking or visual changesAdoption plan— not a new featureChangelog: this is an internal implementation change with no consumer-facing impact, so per the changelog guide I believe
skip-changelogapplies here — matching #9558, #9667 and #9831, which each migrated a component to a function component with no changelog entry. I'm happy to add one if you'd prefer; I can't apply the label myself.QA instructions for reviewer
Open the EuiFilePicker Storybook stories:
initialPromptText, and callsonChangewithnull.disabled.filesprop still controls displayed state, including when the prop reference changes.fullWidthstill inherits from a wrapping<EuiForm fullWidth>.isLoading,isInvalid,compressedanddisplay="default"render as before.Checklist before marking Ready for Review
QA: Tested docs changes— no docs changesfile_pickertests with the snapshot unmodified, and the fullsrc/components/formsuite (38 suites / 382 tests / 222 snapshots) green, plustsc --noEmitand ESLint clean. No new tests added: the migration is behavior-preserving and the existing suite already covers the state and clear-button paths.skip-changelogBreaking changes: label— none