Skip to content

[EuiFlyout] Preserve pixel width on container resize when size is numeric - #9976

Open
clintandrewhall wants to merge 3 commits into
mainfrom
claude/eui-flyout-numeric-clamp-0g0fw8
Open

[EuiFlyout] Preserve pixel width on container resize when size is numeric#9976
clintandrewhall wants to merge 3 commits into
mainfrom
claude/eui-flyout-numeric-clamp-0g0fw8

Conversation

@clintandrewhall

Copy link
Copy Markdown
Contributor

Summary

Closes #9969

What: A resizable EuiFlyout given a numeric size no longer rescales when the flyout container's width changes. It keeps the user's pixel width and is only re-clamped. Container-driven resizes also no longer fire onResize.

Why: When the container width changed, useEuiFlyoutResizable multiplied the current pixel width by the reference-width ratio to preserve the flyout's percentage of the container. For a type="push" flyout this moves both panes on a single container resize — the flyout rescales and the pushed content reflows to match.

Separately, callOnResize is set to true on onMouseUp/onKeyDown and was never reset on the constraint-change path, so these machine-generated rescales called onResize. Consumers that persist the callback value had the user's stored width permanently overwritten by a window resize.

How: Branch on the size type in the constraint-change path of use_flyout_resizable.ts, and reset callOnResize when the reference width actually changed.

setFlyoutWidth((currentWidth) => {
  if (currentWidth && prevRefWidth > 0 && _referenceWidth > 0) {
    // A numeric `size` is a pixel contract — re-clamp rather than rescale.
    if (typeof _size === 'number') {
      return getFlyoutMinMaxWidth(currentWidth);
    }
    const scaleFactor = _referenceWidth / prevRefWidth;
    return getFlyoutMinMaxWidth(currentWidth * scaleFactor);
  }
  ...

Why the fix is conditional, and why it does not conflict with #9683

#9683 fires on the same trigger — the flyout container's width changing — but asks for the opposite outcome: a manually-resized main+child pair should revert to its coded s/m size when it goes stacked. Both asks are valid because they describe different consumer contracts, and typeof size === 'number' is the discriminator:

size prop Contract Behavior on container resize
's' / 'm' / 'l' / 'fill' Percentage semantics Keeps scaling — unchanged by this PR
numeric (e.g. 544) Consumer measured, persisted, and re-supplied pixels Re-clamp only, preserve pixels

Scaling is correct for named sizes because EUI literally defines them as percentages in packages/eui/src/components/flyout/flyout.styles.ts:

s: width 25%
m: width 50%
l: width 75%
   (capped at 90%)

An m flyout that stays at 50% through a container resize is behaving as designed, and the pre-existing code comment said so explicitly ("preserves the flyout's percentage position in both directions"). Removing the scale factor unconditionally would be a silent semantic change for every resizable named-size flyout, and would pre-empt #9683's design space. This PR therefore changes the numeric branch only.

The test preserves the percentage for a named "size" in use_flyout_resizable.test.ts is the regression guard for that: it asserts an m flyout still holds 50% across a reference-width change. It passes both with and without this diff, which is exactly what a guard should do.

Notes for reviewers

  • resizeMode prop deliberately not added. The issue floats an explicit resizeMode: 'pixel' | 'percent' prop as the more discoverable API. This PR implements the inferred (typeof size === 'number') version, which needs no consumer changes and keeps the diff reviewable. Happy to switch to the explicit prop if the team prefers it.
  • The callOnResize reset is gated on the reference width having changed (if (_referenceWidth !== prevRefWidth)) rather than firing unconditionally. An unconditional reset in this branch would risk suppressing the legitimate onResize at the end of a drag when a sibling width or other clamp input changes around the same tick. There is a test covering that a drag-then-release still fires onResize exactly once with the final width.
  • The % round-trip is lossless on this path. referenceWidth comes from useResizeObserver(container, 'width') (which reports borderBoxSize.inlineSize) and flyout.component.tsx converts back with containerRect.width from getBoundingClientRect() — both border-box. This PR does not change the % output mechanism.
  • Known separate follow-up: the container.clientWidth fallback in flyout.component.tsx:365 (used when the ResizeObserver hasn't reported yet) is content-box where the rest of the path is border-box, so it is off by the container's padding on the first frame — including the push padding this component applies itself. That's a real but distinct bug and is intentionally not folded in here.

Reporting consumer

Kibana's Discover document details flyout. Kibana scopes flyouts to the app workspace container (#app-main-scroll), so a window resize, a sidebar resize, or opening the AI Assistant all change referenceWidth; Discover persists the onResize width to localStorage, which is the user-visible half of the bug. Kibana tracks its own consumer-side fixes (the ones that do not need this change) at elastic/kibana#287943.

API Changes

component / parent prop / child change description
EuiFlyout / EuiFlyoutResizable size (numeric) Behavior A numeric size is now treated as a pixel contract: on container resize the flyout is re-clamped rather than rescaled proportionally
EuiFlyout / EuiFlyoutResizable onResize Behavior No longer called for container-driven (non-user) resizes

Screenshots

No visual change for named sizes. For numeric sizes the change is that the flyout stops moving on container resize; see the screenshot in #9969 for the reported behavior.

Impact Assessment

  • 🔴 Breaking changes — None. No API surface changes; the behavior change is scoped to resizable flyouts with a numeric size.
  • 💅 Visual changes — Only for resizable flyouts with a numeric size during a container resize, and in the direction the issue asks for. Named sizes (s/m/l/fill) are untouched.
  • 🧪 Test impact — Added 6 unit tests. No existing test was changed: use_flyout_resizable.test.ts had no coverage of scaling on referenceWidth change (every prior test uses a static referenceWidth), so nothing was weakened to get green.
  • 🔧 Hard to integrate — No consumer changes required.

Impact level: 🟢 Low

Release Readiness

  • Documentation: no doc-visible API change
  • Figma: n/a
  • Migration guide: n/a
  • Adoption plan (new features): bug fix; the reporting consumer is Discover

QA instructions for reviewer

  1. Render a resizable EuiFlyout with type="push", a numeric size (e.g. 544), and a container element narrower than the viewport.
  2. Drag the resize handle to a chosen width and note the pixel value.
  3. Change the container's width (resize the window, or toggle a sidebar beside it).
    • Confirm the flyout stays at the dragged pixel width (re-clamped if it no longer fits within 90% of the container / maxWidth), and that only the pushed content resizes.
    • Confirm onResize does not fire for the container resize.
  4. Repeat with size="m".
  5. Drag-and-release on both.
    • Confirm onResize still fires exactly once with the final width.

Checklist before marking Ready for Review

  • Filled out all sections above
  • QA: Tested light/dark modes, high contrast, mobile, Chrome/Safari/Edge/Firefox, keyboard-only, screen reader — no rendering or styling change
  • QA: Tested in CodeSandbox and Kibana
  • QA: Tested docs changes — no docs changes
  • Tests: Added Jest coverage (verified that the 4 new behavior tests fail against the unfixed hook, and that the 2 regression guards pass either way). Cypress: not added — the existing flyout_resizable.spec.tsx never passes a container and never changes the container width mid-test, so it does not exercise the branch this PR touches and is unaffected. A Cypress case that resizes a real container would be worthwhile, but I could not validate one in my environment (the Cypress binary host is unreachable there), so I left it out rather than push an unverified spec. Happy to add it on request.
  • Changelog: added
  • Breaking changes: Added breaking change label — not a breaking change

Generated by Claude Code

@cla-checker-service

cla-checker-service Bot commented Aug 31, 2026

Copy link
Copy Markdown

💚 CLA has been signed

@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
clintandrewhall pushed a commit that referenced this pull request Aug 31, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ETTN39EVo6iTDT7cztt7T
@clintandrewhall
clintandrewhall requested a balanced review from Copilot August 31, 2026 19:12
@clintandrewhall
clintandrewhall marked this pull request as ready for review August 31, 2026 19:12
@clintandrewhall
clintandrewhall requested a review from a team as a code owner August 31, 2026 19:12

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 callback reset can still allow container-driven onResize calls when the callback identity changes concurrently.

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

Pull request overview

Updates resizable flyouts to preserve numeric pixel widths during container resizing.

Changes:

  • Re-clamps numeric sizes while retaining proportional scaling for named sizes.
  • Prevents most container-driven onResize callbacks.
  • Adds regression tests and changelog entries.
File summaries
File Description
use_flyout_resizable.ts Implements conditional resize behavior.
use_flyout_resizable.test.ts Tests resizing, clamping, and callbacks.
9976.md Documents both fixes.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread packages/eui/src/components/flyout/use_flyout_resizable.ts
clintandrewhall and others added 3 commits August 31, 2026 19:35
…`size`

A resizable flyout given a numeric `size` did not keep the width the user
dragged it to. When the flyout container's width changed, the current pixel
width was multiplied by the reference-width ratio to preserve the flyout's
*percentage* of the container.

That is correct for named sizes — EUI defines `s`/`m`/`l` as 25%/50%/75% in
`flyout.styles.ts` — but a numeric `size` is a pixel contract: the consumer
measured, persisted, and re-supplied a pixel value, and scaling discards
exactly the information they are trying to preserve.

Branch on the `size` type in the constraint-change path: numeric sizes are
re-clamped only, named sizes keep scaling.

Also reset `callOnResize` when the reference width changes. It is set to
`true` on `onMouseUp`/`onKeyDown` and was never reset on this path, so
container resizes fired `onResize` with a machine-generated width and
consumers that persist that value had the user's stored width overwritten.
The reset is gated on the reference width actually changing so it cannot
suppress the legitimate `onResize` at the end of a drag.

Closes #9969

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ETTN39EVo6iTDT7cztt7T
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ETTN39EVo6iTDT7cztt7T
…ring

Resetting `callOnResize` state was not sufficient on its own. The
`onResize` effect can re-run in the same render the container resize
arrives in — for example when a parent rerender also hands down a new
inline `onResize` — and it then reads the render's pre-update
`callOnResize === true`, calling the consumer back with the pre-clamp
width before the state reset lands.

Track the container-driven resize in a ref instead. It is written
synchronously by the constraint effect, which is declared before the
`onResize` effect and so always runs first within a commit, making the
signal visible to the callback effect in the same pass. The ref is
cleared by `onMouseUp`/`onKeyDown` — the only callers that set
`callOnResize` back to `true` — so a user resize following a container
resize is still reported.

Reported by the Copilot reviewer on PR #9976.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ETTN39EVo6iTDT7cztt7T
@clintandrewhall
clintandrewhall force-pushed the claude/eui-flyout-numeric-clamp-0g0fw8 branch from 4cf430f to 40cfb8c Compare August 31, 2026 19:35
@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

History

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

History

@weronikaolejniczak weronikaolejniczak removed the community contribution (Don't delete - used for automation) label Sep 1, 2026
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.

[EuiFlyout] Resizable flyout with a numeric size rescales on container resize instead of preserving the user's pixel width

3 participants