[EuiRange] Migrate to a function component - #9928
[EuiRange] Migrate to a function component#9928franciscodocapa95-stack wants to merge 2 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? |
github-actions
Bot
added
the
community contribution
|
buildkite test this |
📷 2 visual difference(s) foundLook at the visual diff below. If everything is expected, run Approve visual changes to update baselines, re-run the job or make appropriate fixes. See the visual regression testing wiki for more information. Expand to revieweuirange (2 differences)
|
💔 Build Failed
Failed CI StepsHistory |
weronikaolejniczak
added
the
skip-changelog
There was a problem hiding this comment.
nit:
Could you update fireEvent to userEvent in the whole file?
| trackWidth: 0, | ||
| }; | ||
| const theme = useEuiTheme(); | ||
| const [id] = useState(() => propsId || htmlIdGenerator()()); |
There was a problem hiding this comment.
nit: We have a dedicated custom hook for this:
| const [id] = useState(() => propsId || htmlIdGenerator()()); | |
| const generatedId = useGeneratedHtmlId(); | |
| const id = propsId || generatedId; |
| | React.ChangeEvent<HTMLInputElement> | ||
| | React.MouseEvent<HTMLButtonElement> |
There was a problem hiding this comment.
nit:
Let's destructure these types from the import using type keyword instead of using the React namespace. Applies to the rest of the file.
| return thePopover ? thePopover : theRange; | ||
| } | ||
| } | ||
| const thePopover = showInputOnly ? ( |
There was a problem hiding this comment.
nit: Can we just name these popover and range? Adding the prefix the looks weird.
| <EuiInputPopover | ||
| {...inputPopoverProps} | ||
| className={classNames('euiRange__popover', inputPopoverProps?.className)} | ||
| input={theInput!} // `showInputOnly` confirms existence |
There was a problem hiding this comment.
nit: Let's not use bangs or assertions. Those are essentially telling the TS that we know better. Let's fix the underlying issue instead.
| fullWidth={fullWidth} | ||
| isOpen={isPopoverOpen} | ||
| closePopover={closePopover} | ||
| disableFocusTrap={true} |
There was a problem hiding this comment.
nit:
This can simply be:
| disableFocusTrap={true} | |
| disableFocusTrap |
|
blocking: The VRTs no longer show the range component. We should fix that! |
💚 Build Succeeded
|
2 participants
Summary
Migrates
EuiRangefrom a class component to a function component.useFormContext()anduseEuiTheme().EuiRangeClassexport and class-instance ref usage.Closes #9494.
API Changes
There are no changes to the documented
EuiRangeAPI.The internal
EuiRangeClassexport and class-instance ref access are removed as part of the class-to-function migration, as discussed in #9494.Screenshots
No visual changes are expected.
The migrated component was tested locally in Storybook and compared with the current production documentation. Slider/input synchronization, popover behavior, ticks, levels, highlight, tooltip, and keyboard interaction remained consistent.
Impact Assessment
Impact level: 🟢 Low
The migration preserves the existing DOM structure, class names, styles, ARIA attributes, documented props, and snapshots.
Release Readiness
Documentation: No documentation changes requiredFigma: No design changesMigration guide: No documented API or consumer migration requiredAdoption plan: Not applicable; this is an internal component implementation migrationQA instructions for reviewer
InputWithPopoverStorybook story focuses the real numeric input.Checklist before marking Ready for Review
Changelog: No changelog required, as confirmed in [EuiRange] Migrate from class to function component #9494Breaking changes: No breaking change label requiredValidation
EuiRangetests passedEuiRangetests passedgit diff --checkpassedReviewer checklist