Skip to content

[EuiSuperSelect] Migrate to function component - #9972

Open
mehuljariwala wants to merge 2 commits into
elastic:mainfrom
mehuljariwala:fix/9485-super-select-hooks
Open

[EuiSuperSelect] Migrate to function component#9972
mehuljariwala wants to merge 2 commits into
elastic:mainfrom
mehuljariwala:fix/9485-super-select-hooks

Conversation

@mehuljariwala

Copy link
Copy Markdown
Contributor

Summary

  • Converts EuiSuperSelect from a generic class component to a generic function component using useState, useRef, and useEffect.
  • Preserves the existing public props, default values, controlled isOpen behavior, option focus order, disabled-option skipping, and control-button focus restoration.
  • Keeps the mount guard around the nested animation frames so a pending focus callback does not run after unmount.
  • Keeps the screen-reader description ID stable for the lifetime of the component.
  • Updates the type-only test directives to match the more precise JSX diagnostic locations produced by a generic function component; the invalid generic values remain rejected.

Fixes #9485.

API Changes

component / parent prop / child change description
EuiSuperSelect Public API None Implementation-only migration; props, defaults, DOM structure, and behavior are unchanged.

Screenshots

Not applicable. This is an implementation-only refactor with no intended visual or DOM changes; all 13 focused snapshots remain unchanged.

Impact Assessment

  • 🔴 Breaking changes — None.
  • 💅 Visual changes — None intended.
  • 🧪 Test impact — Type-only @ts-expect-error comments moved to the prop diagnostics emitted for a function component. Runtime assertions and snapshots are unchanged.
  • 🔧 Hard to integrate — No integration work expected.

Impact level: 🟢 Low

Release Readiness

QA instructions for reviewer

From packages/eui:

yarn build:workspaces
yarn test-unit super_select --runInBand
yarn test-cypress --spec src/components/form/super_select/super_select.spec.tsx
yarn lint

The Cypress spec checks the user-visible behaviors most at risk in this migration:

  • Open the popover with Arrow Up, Arrow Down, Space, and Enter.
  • Select the focused option with Enter and Tab.
  • Close with Escape without changing the selection.
  • Confirm arrow navigation stops at the first and last option.
  • Confirm disabled options are skipped.
  • Confirm form-row focus styling remains active while navigating the dropdown.

Local validation performed with Node 24.19.0 and React 18:

  • Focused Jest: 2 suites, 32 tests, and 13 snapshots passed.
  • Focused Cypress: 6 tests passed in headless Chrome 151. A cold first run passed overall after one retry in the opening-key test; an immediate clean rerun passed 6/6 with no retries or screenshots.
  • Package TypeScript: passed.
  • Full yarn lint: passed with zero errors (repository-wide existing warnings remain).
  • Pre-push yarn test-staged: 368 suites, 5,302 tests, and 2,754 snapshots passed; 1 suite and 21 tests were skipped by the repository configuration.

Checklist before marking Ready for Review

  • Filled out all sections above
  • QA: Covered keyboard-only behavior in headless Chrome through the existing Cypress component suite. No visual output changed.
  • QA: CodeSandbox and Kibana testing are not needed for this internal refactor with unchanged API, DOM snapshots, and browser behavior.
  • QA: No documentation changes.
  • Tests: Updated the generic typing assertions and ran the existing Jest and Cypress coverage for EuiSuperSelect. No VRT was run locally because there is no visual change.
  • Changelog: No consumer-facing change; this PR should use skip-changelog.
  • Breaking changes: Not applicable.

Reviewer checklist

  • Approved Impact Assessment — Acceptable to merge given the consumer impact.
  • Approved Release Readiness — Docs, Figma, and migration info are sufficient to ship.

@mehuljariwala
mehuljariwala requested a review from a team as a code owner August 31, 2026 06:16
@github-actions

Copy link
Copy Markdown

👋 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 github-actions Bot added the community contribution (Don't delete - used for automation) label Aug 31, 2026
: undefined;

return (
<RenderWithEuiStylesMemoizer>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: We don't need to do RenderWithEuiStylesMemoizer in a functional component, we can use the hook useEuiMemoizedStyles.

@weronikaolejniczak

Copy link
Copy Markdown
Contributor

buildkite test this

@weronikaolejniczak weronikaolejniczak added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Sep 2, 2026
@weronikaolejniczak
weronikaolejniczak requested a lite review from Copilot September 2, 2026 13:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new mount guard sets isMounted only in useEffect, which can prevent initial option focusing/onFocus in fast post-mount interactions and risks a behavioral regression.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Refactors EuiSuperSelect from a generic class component to a generic function component using React hooks, aiming to preserve existing API and keyboard/focus behavior while modernizing the implementation.

Changes:

  • Migrated EuiSuperSelect to a hook-based function component (useState, useRef, useEffect) while keeping public props and DOM structure stable.
  • Preserved focus management behaviors (initial option focus, disabled-option skipping, focus restoration) with mount guarding for async RAF focus.
  • Updated type-only tests to align @ts-expect-error directives with JSX prop-level diagnostics.
File summaries
File Description
packages/eui/src/components/form/super_select/super_select.tsx Reimplements EuiSuperSelect as a function component with hook-based state, refs, and lifecycle equivalents.
packages/eui/src/components/form/super_select/super_select.test.tsx Adjusts TS typing assertion comments to match the new JSX error locations.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +248 to +252
useEffect(() => {
isMounted.current = true;
if (isOpen) {
openPopover();
}
@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

@weronikaolejniczak weronikaolejniczak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The overall migration looks fine but we will break Kibana with this update if we don't forwardRef and useImperativeHandle({ openPopover, closePopover }). See https://github.com/elastic/kibana/blob/main/x-pack/platform/packages/shared/response-ops/alerts-filters-form/components/alerts_solution_selector.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community contribution (Don't delete - used for automation) skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EuiSuperSelect] Migrate from class to function component

3 participants