[Flyout Manager] Expose the close source as a parameter to the onClose#9716
Open
tsullivan wants to merge 9 commits into
Open
[Flyout Manager] Expose the close source as a parameter to the onClose#9716tsullivan wants to merge 9 commits into
tsullivan wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the EuiFlyout close callback API to optionally include close-source metadata (onClose(event, meta?: { reason })), enabling consumers to distinguish between close causes (close button, Escape, outside click, and managed-flyout navigation closes). It also exports supporting close-related types as part of the public flyout API.
Changes:
- Added
EuiFlyoutCloseReason+EuiFlyoutCloseMetatypes and exported them (along withEuiFlyoutCloseEvent) from the flyout public index. - Updated
EuiFlyoutComponentto forward a closereasonfor Escape, outside click, and the default close button. - Updated managed flyouts to pass navigation close reasons (
navigation-back/navigation-cascade) and added/updated unit tests + changelog entry.
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/eui/src/components/flyout/types.ts | Adds new close reason/meta types to support passing close-source info. |
| packages/eui/src/components/flyout/flyout.component.tsx | Extends onClose signature and threads close reason through core close paths. |
| packages/eui/src/components/flyout/manager/flyout_managed.tsx | Threads meta through managed flyout closes and adds navigation-specific reasons. |
| packages/eui/src/components/flyout/manager/flyout_managed.test.tsx | Adds assertions for managed navigation close reasons. |
| packages/eui/src/components/flyout/flyout.test.tsx | Adds unit tests for base flyout close reasons (close button, Escape). |
| packages/eui/src/components/flyout/index.ts | Exports new close-related types from the public flyout barrel. |
| packages/eui/changelogs/upcoming/9715.md | Documents the additive onClose(event, meta) API change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+89
to
+93
| * The optional second `meta` argument describes why the flyout closed via | ||
| * `meta.reason` (e.g. `'close-button'`, `'escape'`, `'outside-click'`, and, | ||
| * for managed flyouts, `'navigation-back'` or `'navigation-cascade'`). | ||
| */ | ||
| onClose: (event: EuiFlyoutCloseEvent) => void; | ||
| onClose: (event: EuiFlyoutCloseEvent, meta?: EuiFlyoutCloseMeta) => void; |
Comment on lines
307
to
311
| wasRegisteredRef.current = false; // Prevent cleanup from double-firing onClose | ||
| if (onCloseCallbackRef.current) { | ||
| const event = e || new MouseEvent('click'); | ||
| onCloseCallbackRef.current(event); | ||
| onCloseCallbackRef.current(event, meta); | ||
| } |
Member
Author
There was a problem hiding this comment.
duplicate feedback, #9716 (comment) is the same
Comment on lines
+1068
to
+1073
| it("passes reason 'escape' when the Escape key is pressed", () => { | ||
| const onClose = jest.fn(); | ||
| render(<EuiFlyout onClose={onClose} />); | ||
|
|
||
| fireEvent.keyDown(document, { key: 'Escape' }); | ||
|
|
tkajtoch
reviewed
Jun 5, 2026
tkajtoch
left a comment
tkajtoch
left a comment
Member
There was a problem hiding this comment.
The changes look good, thank you for the PR! @tsullivan Before I approve, could you please review Copilot's comments please?
tsullivan
commented
Jun 5, 2026
| // flyouts it removes so the managed flyout can report `navigation-back`; any | ||
| // other removal (e.g. closeAllFlyouts cascade) leaves no stamp and defaults to | ||
| // `navigation-cascade`. Kept off reducer state because it is a per-close | ||
| // annotation, not persistent state. |
Member
Author
There was a problem hiding this comment.
This approach may feel a bit overcomplicated, but it serves two worthwhile purposes:
- A transient "reason-channel" in the store is the correct approach because the store is the only place that knows why a flyout was removed — whether it was via
goBack(),closeAllFlyouts()cascade, or some other path. The managed flyout component can't reliably infer the reason from the event alone or from mount/unmount timing; it needs the store to explicitly signal the cause at the moment of removal. - The reason channel prepares the code for the history management refactor because after the refactor extracts history management to a package outside of EUI, that package will call EUI's store methods (like
goBack()) to navigate. The upcoming history package will communicate why a flyout closed by passing the reason throughstore.goBack(), and EUI flows the reason through thependingCloseMetachannel.
Comment on lines
+264
to
+276
| goBack: () => { | ||
| // Capture which flyouts goBack removes so they report `navigation-back` | ||
| // (vs. the default `navigation-cascade`). dispatch updates state | ||
| // synchronously, so a before/after diff reliably identifies them. | ||
| const removedBefore = currentState.flyouts.map((f) => f.flyoutId); | ||
| dispatch(goBackAction()); | ||
| const remaining = new Set(currentState.flyouts.map((f) => f.flyoutId)); | ||
| removedBefore.forEach((flyoutId) => { | ||
| if (!remaining.has(flyoutId)) { | ||
| pendingCloseMeta.set(flyoutId, { reason: 'navigation-back' }); | ||
| } | ||
| }); | ||
| }, |
| * internally by managed flyouts to report the correct close reason; defaults | ||
| * to `navigation-cascade` when nothing was stashed. | ||
| */ | ||
| consumeCloseMeta: (flyoutId: string) => EuiFlyoutCloseMeta | undefined; |
Member
Author
There was a problem hiding this comment.
The EUI Flyout System is marked as a beta feature, so additive behavior should be OK.
Comment on lines
+1056
to
+1078
| describe('onClose reason', () => { | ||
| it("passes reason 'close-button' when the close button is clicked", () => { | ||
| const onClose = jest.fn(); | ||
| const { getByTestSubject } = render(<EuiFlyout onClose={onClose} />); | ||
|
|
||
| fireEvent.click(getByTestSubject('euiFlyoutCloseButton')); | ||
|
|
||
| expect(onClose).toHaveBeenCalledWith(expect.anything(), { | ||
| reason: 'close-button', | ||
| }); | ||
| }); | ||
|
|
||
| it("passes reason 'escape' when the Escape key is pressed", () => { | ||
| const onClose = jest.fn(); | ||
| render(<EuiFlyout onClose={onClose} />); | ||
|
|
||
| fireEvent.keyDown(window, { key: 'Escape' }); | ||
|
|
||
| expect(onClose).toHaveBeenCalledWith(expect.anything(), { | ||
| reason: 'escape', | ||
| }); | ||
| }); | ||
| }); |
Member
Author
There was a problem hiding this comment.
'outside-click' does not originate in EUI code, and the test path is not reachable in jsdom. The functionality is supported by the third-party react-focus-on library, which EuiFocusTrap wraps. Looking at react-focus-on/dist/es5/Effect.js, its document-level mousedown listener:
- is only attached once the focus lock has activated (
activeNodeis set), and - activation is driven by the focus-lock "medium" sidecar reacting to real focus events.
In jsdom this activation path doesn't reliably run, so the onClickOutside callback that carries the 'outside-click' reason never fires.
Collaborator
💚 Build Succeeded
History
|
Collaborator
💚 Build SucceededHistory
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What: Adds an optional second
metaargument toEuiFlyout'sonClosecallback —onClose(event, { reason })— so consumers can tell why a flyout closed. Thereasonis one of'close-button','escape','outside-click', and, for managed flyouts,'navigation-back'or'navigation-cascade'. Also exports the supporting types (EuiFlyoutCloseEvent,EuiFlyoutCloseReason,EuiFlyoutCloseMeta) —EuiFlyoutCloseEventwas previously not part of the public API.Why: Managed-flyout consumers had no way to distinguish close sources at the event level. The tab‑switch cascade close and the back‑button close both fired an identical synthetic
MouseEvent('navigation'). Kibana Discover's traces experience needs to preserve restorable flyout state on a tab switch but clear it on a back‑button click — impossible without fragile workarounds (inferring the source from React mount/unmount timing, or sniffingevent.type === 'navigation', which is an internal detail and not public API).How:
EuiFlyoutCloseReasonunion +EuiFlyoutCloseMetainterface inflyout/types.ts.flyout.component.tsx: widened theonClosesignature;handleClosenow forwards a reason (defaulting to'close-button'), with'escape'from the Escape handler,'outside-click'from the overlay/outside‑click handler, and'close-button'from the close button.flyout_managed.tsx: threadsmetathrough theonClosewrapper, and emits'navigation-cascade'on the parent‑unmount cascade path and'navigation-back'on the back‑button path.flyout/index.ts.Non‑obvious decision for reviewers: the synthetic
MouseEvent('navigation')is intentionally kept as the first argument so any existing consumer relying onevent.type === 'navigation'is not broken; they can migrate tometa.reasonincrementally. The secondmetaargument is optional, so existing(event) => voidhandlers remain valid and behaviorally unchanged.API Changes
EuiFlyout/EuiFlyoutComponentPropsonClose(event, meta?: { reason: EuiFlyoutCloseReason }) => void. Optional second arg; existing(event) => voidhandlers are unaffected.EuiFlyoutCloseReason'close-button' | 'escape' | 'outside-click' | 'navigation-back' | 'navigation-cascade'.EuiFlyoutCloseMeta{ reason: EuiFlyoutCloseReason }passed asonClose's second argument.EuiFlyoutCloseEventScreenshots
No visual changes — this is an API/behavior‑only change (a new optional callback argument). Nothing about flyout rendering, markup, or styling changes, so there is nothing to show before/after.
Impact Assessment
metaargument is optional; existingonClose={(event) => …}handlers compile and behave exactly as before.reasonfor close button, Escape, and the managednavigation-back/navigation-cascadepaths.Impact level: 🟢 Low
A couple of things to fill in before you post:
Fixes #<issue>in the Summary if there's a tracking issue for the Discover request.changelogs/upcoming/9715.md(placeholder). Rename it to the real PR number once it's open, since the filename becomes the PR link.Want me to also draft a short, conventional commit body for the squash-merge message?
─── Write your reply below this line ──────────────────────────
QA instructions for reviewer
Checklist before marking Ready for Review
breaking changelabel (if applicable)Reviewer checklist