Skip to content

[eslint-plugin] Add button-group-no-invalid-children rule - #9849

Open
mgadewoll wants to merge 12 commits into
elastic:mainfrom
mgadewoll:buttongroup/eslint-no-invalid-children
Open

[eslint-plugin] Add button-group-no-invalid-children rule#9849
mgadewoll wants to merge 12 commits into
elastic:mainfrom
mgadewoll:buttongroup/eslint-no-invalid-children

Conversation

@mgadewoll

@mgadewoll mgadewoll commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Note

This PR is related to #9845 which adds the new Children API on EuiButtonGroup.

Examples

✅ valid

// direct buttons
<EuiButtonGroup legend="Actions">
  <EuiButton>Save</EuiButton>
  <EuiButtonEmpty color="text">Cancel</EuiButtonEmpty>
</EuiButtonGroup>

// EuiToolTip
<EuiButtonGroup legend="Actions">
  <EuiButton>Save</EuiButton>
  <EuiToolTip content="Delete">
    <EuiButtonIcon iconType="trash" />
  </EuiToolTip>
</EuiButtonGroup>

// conditional rendering
<EuiButtonGroup legend="Actions">
  <EuiButton>Save</EuiButton>
  {canDelete && <EuiButtonIcon iconType="trash" />}
</EuiButtonGroup>

// EuiCopy
<EuiButtonGroup legend="Actions">
  <EuiCopy textToCopy="text">
    {(copy) => <EuiButton onClick={copy}>Copy</EuiButton>}
  </EuiCopy>
</EuiButtonGroup>

// EuiPopover
<EuiButtonGroup legend="Actions">
  <EuiPopover
    button={
      <EuiToolTip content="More options">
        <EuiButtonIcon iconType="boxesVertical" />
      </EuiToolTip>
    }
    isOpen={isOpen}
    closePopover={closePopover}
  >
    Panel content
  </EuiPopover>
</EuiButtonGroup>

❌ invalid

// non-button element
<EuiButtonGroup legend="Actions">
  <div>Not a button</div>
</EuiButtonGroup>

// invalid EuiToolTip children
<EuiButtonGroup legend="Actions">
  <EuiToolTip content="tip">
    <EuiFlexGroup>...</EuiFlexGroup>
  </EuiToolTip>
</EuiButtonGroup>

// invalid EuiPopover button
<EuiButtonGroup legend="Actions">
  <EuiPopover button={<div>Not a button</div>}>
    Panel content
  </EuiPopover>
</EuiButtonGroup>

Additional information

The EuiButtonGroup redesign adds a Children API (PR) as an alternative to the existing options-array API. With this API, consumers compose button components directly as JSX children rather than passing a configuration array. Because JSX children are freeform, the rule provides a lint-time guardrail that catches invalid compositions without requiring a runtime throw.

What the rule validates

The rule validates direct children for variant="default": EuiButton, EuiButtonEmpty, EuiButtonIcon. (later iterations will support more variant values). Additionally it validates allowed the following allowed wrapper components: EuiPopover, EuiToolTip and EuiCopy.

Note

This rule will be extended with the following implementation iterations as there are different requirements for the button group depending on some props.

API Changes

⚪ No API changes

Screenshots

Screenshot 2026-07-28 at 11 45 02

Impact Assessment

Note: Most PRs should be tested in Kibana to help gauge their Impact before merging.

  • 🔴 Breaking changes — What will break? How many usages in Kibana/Cloud UI are impacted?
  • 💅 Visual changes — May impact style overrides; could require visual testing. Explain and estimate impact.
  • 🧪 Test impact — May break functional or snapshot tests (e.g., HTML structure, class names, default values).
  • 🔧 Hard to integrate — If changes require substantial updates to Kibana, please stage the changes and link them here.

Impact level: 🟢 None

Release Readiness

  • Documentation: {link to docs page(s)}
  • Figma: {link to Figma or issue}
  • Migration guide: {steps or link, for breaking/visual changes or deprecations}
  • Adoption plan (new features): {link to issue/doc or outline who will integrate this and where}

QA instructions for reviewer

  • CI passes
  • verify locally the rule applies and flags invalid usages
    • checkout the PR
    • run yarn workspace @elastic/eslint-plugin-eui build on root
    • run yarn workspace @elastic/eui build:workspaces on root
    • add testing code example (e.g. see above) and/or update button_group_children.stories.tsx and run eslint check with yarn workspace @elastic/eui lint on root

Checklist before marking Ready for Review

Reviewer checklist

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

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.

Pull request overview

Adds a new @elastic/eui ESLint rule to statically validate EuiButtonGroup’s new Children API usage, helping consumers avoid composing unsupported child elements and wrapper patterns.

Changes:

  • Implement button-group-no-invalid-children rule, validating allowed direct children and supported wrappers (including special handling for EuiPopover’s button prop).
  • Add JSX child-collection utilities to expand/resolve common composition patterns (fragments, conditionals, arrays, identifiers, local arrow components, and .map() callbacks).
  • Add comprehensive unit tests and document the new rule in the plugin README; register the rule in the plugin’s exported config.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/eslint-plugin/src/utils/is_fragment.ts New helper to identify <Fragment> / <React.Fragment> opening elements.
packages/eslint-plugin/src/utils/flat_map.ts New flatMap shim used by rule utilities.
packages/eslint-plugin/src/utils/collect_jsx_children.ts New utility to expand JSX children into concrete JSXElements for validation.
packages/eslint-plugin/src/rules/button_group_no_invalid_children.ts New ESLint rule implementation for EuiButtonGroup Children API validation.
packages/eslint-plugin/src/rules/button_group_no_invalid_children.test.ts Unit tests covering valid/invalid child patterns and wrapper handling.
packages/eslint-plugin/src/index.ts Registers the new rule and enables it in recommended.
packages/eslint-plugin/README.md Adds end-user documentation and examples for the new rule.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/eslint-plugin/src/utils/is_fragment.ts
Comment thread packages/eslint-plugin/src/utils/collect_jsx_children.ts Outdated
Comment thread packages/eslint-plugin/src/utils/flat_map.ts
@mgadewoll mgadewoll changed the title [ESLint] Add button-group-no-invalid-children rule [eslint-plugin] Add button-group-no-invalid-children rule Jul 30, 2026
@mgadewoll
mgadewoll marked this pull request as ready for review July 30, 2026 13:08
@mgadewoll
mgadewoll requested a review from a team as a code owner July 30, 2026 13:08

@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.

Overall, the rule looks good 👍🏻 impressive test coverage, took me some time to go over all cases. This is a new API that doesn't have usage in Kibana so it's hard to discuss what cases have to be covered. I only noticed one inconsistency and that is:

EuiPopover button prop can have a custom component <SaveButton />, it gets invalidPopoverButton that doesn't have the hint about suppressing a comment. Shouldn't it use invalidUnresolvableChild (or could be invalidUnresolvablePopoverButton)? Same with EuiToolTip inside EuiPopover case. We can mention it inside "Custom button wrapper components" as well.

One potential friction point is the rule returns invalidChild for all Eui-prefixed components that might be defined by the consumer (e.g. EuiCustomWrapper) instead of invalidUnresorvableChild and they don't see the suppress hint.

Comment thread packages/eslint-plugin/src/rules/button_group_no_invalid_children.ts Outdated
Comment thread packages/eslint-plugin/src/rules/button_group_no_invalid_children.ts Outdated

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.

non-blocking suggestion: This utility could be simplified to just the traversal (fragment expansion, conditionals, arrays, &&) and it could be reused in callout_prefer_props_for_content.ts and tooltip_no_interactive_content.ts. The scope lookups could stay in the rule's source directly. The only functional difference between the rules is the && behavior because in both callout and tooltip rules we check both sides, this utility checks only the right.

But it's not something I'll push for, this is fine as-is.

@mgadewoll mgadewoll Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's correct. I mainly didn't want to update other rules in this PR to not potentially affect other code outside the scope. 😅
But if we're ok adding it here to be complete, we can refactor it 👍 It needs a bit more refactoring to ensure it's truly generic. I added a proposal refactor (commit) that walks the children and applies custom checks per rule. I think a simple shared collector wasn't enough because the three rules have different needs (button group should resolve variable references via scope, callout checks non-JSXElement content like plain text and literals). Let me know what you think, in case we can also revert it.

@mgadewoll

Copy link
Copy Markdown
Contributor Author

EuiPopover button prop can have a custom component <SaveButton />, it gets invalidPopoverButton that doesn't have the hint about suppressing a comment. Shouldn't it use invalidUnresolvableChild (or could be invalidUnresolvablePopoverButton)? Same with EuiToolTip inside EuiPopover case. We can mention it inside "Custom button wrapper components" as well.

One potential friction point is the rule returns invalidChild for all Eui-prefixed components that might be defined by the consumer (e.g. EuiCustomWrapper) instead of invalidUnresorvableChild and they don't see the suppress hint.

Ah yes, thank's for the catch! Those should show the hint indeed to be useful. Updated in 5590292

- makes the util more generic and universally useable by rules to traverse children; provides means to customize by
@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

History

cc @mgadewoll

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

History

cc @mgadewoll

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants