[Flyout Manager] Expose the close source as a parameter to the onClose#9716
[Flyout Manager] Expose the close source as a parameter to the onClose#9716tsullivan wants to merge 11 commits into
Conversation
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.
tkajtoch
left a comment
tkajtoch
left a comment
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?
| // 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. |
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.
| * internally by managed flyouts to report the correct close reason; defaults | ||
| * to `navigation-cascade` when nothing was stashed. | ||
| */ | ||
| consumeCloseMeta: (flyoutId: string) => EuiFlyoutCloseMeta | undefined; |
There was a problem hiding this comment.
The EUI Flyout System is marked as a beta feature, so additive behavior should be OK.
| 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', | ||
| }); | ||
| }); | ||
| }); |
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.
2 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuipopover (1 difference)
euidatagrid (1 difference)
|
💔 Build Failed
Failed CI StepsHistory
|
💔 Build Failed
Failed CI StepsHistory
|
|
@tkajtoch I've responded to the review from Copilot. I've also fixed a bug since you've last viewed. |
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'.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').How:
EuiFlyoutCloseReasonunion +EuiFlyoutCloseMetainterface inflyout/types.ts.flyout.component.tsx: widened theonClosesignature;handleCloseforwards an optional reason —'escape'from the Escape handler,'outside-click'from the overlay/outside‑click handler, and'close-button'from the close button (base and menu). When no reason applies,metais omitted entirely rather than guessing.flyout_managed.tsx: threadsmetathrough theonClosewrapper. For closes that originate outside the flyout (a flyout removed from the manager store while still mounted, or swept up by a cascade), the reason comes from a store-level reason channel rather than being hardcoded at the call site.store.ts:goBack()stamps the flyout(s) it removes with{ reason: 'navigation-back' }(computed via a before/after diff ofstate.flyouts) into a transientpendingCloseMetamap. Managed flyouts read-and-clear it via a newconsumeCloseMeta(flyoutId)store method, defaulting to'navigation-cascade'when nothing was stamped. So'navigation-back'is emitted only whengoBack()actually caused the removal; every other store-driven removal (e.g.closeAllFlyouts) is'navigation-cascade'.Why the store owns the reason (not the component): only the store knows why a flyout was removed —
goBack()vs.closeAllFlyouts()vs. a singlecloseFlyout(). An earlier approach inferred "removed while mounted ⇒navigation-back" from the component, which mislabeled flyouts a cascade swept up while they were still mounted (e.g. a backgrounded main flyout in a multi-session stack reportednavigation-backwhen another flyout's close cleared the group). Moving the decision to the store fixes that and removes reliance on React mount/unmount timing.Forward compatibility: this channel is the handoff point for the upcoming extraction of history management into a separate package. That package will drive navigation by calling EUI store methods (e.g.
goBack()) and rely on this mechanism to communicate the close reason — it never needs to touchpendingCloseMetaor know how the reason reaches the component. EUI stamps and flows the reason internally.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.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.EuiFlyoutCloseEventFlyoutManagerStoreconsumeCloseMeta(flyoutId: string) => EuiFlyoutCloseMeta | undefined. Reads and clears a one-shot close reason stamped by the store (e.g. bygoBack). Primarily internal plumbing for managed flyouts.Screenshots
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, the managednavigation-back/navigation-cascadepaths, and the store-level reason channel (goBackstamping +consumeCloseMeta).Impact level: 🟢 Low
QA instructions for reviewer
onCloseto logmeta.reason.close-button.escape; click outside (overlay) → logsoutside-click.close-button, while the child and the backgrounded "Park hours" lognavigation-cascade(notnavigation-back).navigation-back.Checklist before marking Ready for Review
breaking changelabel (if applicable)Reviewer checklist