eui icon indicating copy to clipboard operation
eui copied to clipboard

Add EuiFlyoutChild for Grouped Flyouts

Open tsullivan opened this issue 5 months ago • 2 comments

EuiFlyoutChild: A Secondary "Child" Flyout with Dynamic Responsive Stacking

This PR introduces the EuiFlyoutChild component, a new addition to the EUI flyout system, designed to allow for side-by-side (grouped) flyout panels.

https://github.com/user-attachments/assets/1f311949-bab1-41d1-a5ad-1bc7ace390da

Feature Overview

The EuiFlyoutChild component enables a secondary flyout panel to be opened adjacent to a primary EuiFlyout.

Key Behaviors:

  1. Side-by-Side Layout: On wider viewports, the child flyout appears to the left edge of its parent EuiFlyout.
  2. Stacked Layout: On narrower viewports, the child flyout stacks on top of its parent, occupying the full width of the parent.
  3. Dynamic Stacking Breakpoint: The transition point between side-by-side and stacked layouts is dynamically calculated based on the combined widths of the parent and child flyouts.
    • The stacking breakpoint is the sum of the parent flyout's effective width and the child flyout's effective width.
    • For example:
      • If Parent is size 'm' and Child is size 's', the breakpoint is theme.breakpoint.m + theme.breakpoint.s.
      • If Parent is size 's' and Child is size 's', the breakpoint is theme.breakpoint.s + theme.breakpoint.s.
  4. Size Restrictions:
    • EuiFlyoutChild supports sizes 's' (small) and 'm' (medium).
    • If the parent EuiFlyout is size 'm', the EuiFlyoutChild can only be size 's'.
    • If the parent EuiFlyout is size 's', the EuiFlyoutChild can be size 's' or 'm'.

Implementation Details

  • Component Structure (eui_flyout_child.tsx):
    • Manages its own responsive logic by listening to window resize events to get the current viewport width (window.innerWidth).
    • Calculates the stackingBreakpointValue (numeric pixel value) by summing the theme-defined pixel widths corresponding to the parent's and child's sizes.
    • Determines isAboveStackingBreakpoint (boolean) by comparing window.innerWidth to stackingBreakpointValue. This state variable controls whether to apply side-by-side or stacked positioning styles.
  • Context Usage:
    • Uses EuiFlyoutContext to get the size of the parent EuiFlyout.
  • Validation:
    • Ensures EuiFlyoutChild is used within an EuiFlyout.
    • Requires an EuiFlyoutBody as a child.
    • Enforces the size restriction (parent 'm' cannot have child 'm').

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • [ ] Checked in both light and dark modes
    • [ ] Checked in both MacOS and Windows high contrast modes
    • [ ] Checked in mobile
    • [ ] Checked in Chrome, Safari, Edge, and Firefox
    • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • [ ] A changelog entry exists and is marked appropriately.
    • [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • [ ] If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

tsullivan avatar Jun 09 '25 19:06 tsullivan

I'm in the process of reviewing this work

tkajtoch avatar Jun 24 '25 17:06 tkajtoch

@tkajtoch I've spent a little bit of time trying to make animation effects work for the child flyout. The requirements I was attempting are:

  • When animating, the child flyout should slide in and out of view from "under" the parent flyout. However, once the child is finished animating it will need a z-index than the parent flyout, because of our responsive "stacked layout" rules.
  • We want the child flyout to animate when opening and closing, but not when it gets a layout change due to responsive rules (maybe, depending on feedback).

Unfortunately, this is a bit beyond my ability. As this PR should be for an MVP, I hope we can hold off on the animation requirements until a future PR, when someone with more CSS expertise can attempt it.

cc @ryankeairns @JasonStoltz

tsullivan avatar Jun 24 '25 18:06 tsullivan

We can definitely help animating child flyout transitions @tsullivan! Is having them animated a requirement, though?

tkajtoch avatar Jun 24 '25 18:06 tkajtoch

We can definitely help animating child flyout transitions @tsullivan! Is having them animated a requirement, though?

@tkajtoch I'm not hearing that it's a "hard" requirement. I'm hearing that designers want to get their hands on the MVP and evaluate / give feedback whether animation is needed, and to what extent.

From @alexmarhaba:

small nitpick: the animation when the flyout disappears on resize feels a bit strange, as if it's being pushed out of the screen. Is there a a way to make it simply disappear? (no sliding animation)

when opening a child flyout, it's nice to have it slide out of the parent flyout to highlight the "parent / child" relationship, but I'd defer to @ryankeairns

From @ryankeairns:

+1 to animating the child. ... that can be a bit of a rabbit hole to get 'just right' so a rough animation will do initially. Then, we can refine it once we have something to click around on / test.

From @petrklapka:

I like the animation on resize, it "informs" me what is happening. I'll only +0.5 an animation on the child opening though. There's a nice "crispness" to it opening instantly, a perception of performance if you will, that might get lost if it is animated. I'd recommend a "fast" animation of it opening. You know that scene in Star Wars when the door closes crazy fast? Like that!

From @ryankeairns:

Agreed. Almost unnoticeable; prioritize 'snappiness'

tsullivan avatar Jun 24 '25 18:06 tsullivan

Thanks Tim!

Regarding animation

  • I'm curious if we tried/should apply the same flyout animation to the child as already exists for the parent?
  • Personally, I am ok with coming back to animation of the child if that's the consensus from the group

One visual 'bug' In my initial review, I noticed there is a gradient that appears over the child footer:

CleanShot 2025-06-24 at 15 08 37@2x

ryankeairns avatar Jun 24 '25 22:06 ryankeairns

The close button is layering behind the child flyout panel on the small breakpoint; probably need to crank up the z-index

ryankeairns avatar Jun 24 '25 22:06 ryankeairns

Ok, so we got this out of the gate by simplifying the size options... now, some follow up questions:

  • ~As it stands, is the parent allowed to be any size / is the only the child technically limited to s, m?~ Update: tested and now see that the presence of a child enforces the parent size to be either s or m
  • We know, in the end, the child will need to be rather large to contain a 'tool' like the Analyzer graph... how do you suppose we approach or enhance for this option? For example, we may do something like add a fill option for the child size (e.g. s, m, fill)
  • Also 'in the end', we will need to support custom/specific sizes for the main flyout... which means whatever the sizes provided, we will need to handle and adapt

So that we are all on the same page, should we handle these requirements in a subsequent PR(s)? And, if so, perhaps it is best to keep this new functionality undocumented until these (and potentially other) enhancements are made?

ryankeairns avatar Jun 24 '25 22:06 ryankeairns

https://github.com/elastic/eui/pull/8771#issuecomment-3002033086

Regarding animation I'm curious if we tried/should apply the same flyout animation to the child as already exists for the parent?

@ryankeairns: Yes, I've spent a few hours on this. There are unique aspects of the child flyout that make it a bit more challenging:

  • When animating open/closed, the child flyout should appear to slide "under" the parent flyout. However, once the animation finishes, the child flyout should conceptually "stacked above" the parent flyout, in order to implement the responsive rules of using a "stacked layout" that puts the child flyout on top of the parent when the screen size is too small to show both.

  • The child and parent flyouts have different "stacking contexts" due to their positioning styles, which makes the z-index logic more complicated. This is the main thing I struggle with.

  • We haven't settled on the desired behavior of whether the child will animate when responsively switching the layout between "stacked" and "side-by-side." This is more of a discussion item that is still pending.

I've captured the ongoing discussion here: https://github.com/elastic/eui/pull/8771#issuecomment-3001458256

One visual 'bug' In my initial review, I noticed there is a gradient that appears over the child footer:

Wow, great catch! Fixed in f02a3b93c


https://github.com/elastic/eui/pull/8771#issuecomment-3002049342

The close button is layering behind the child flyout panel on the small breakpoint; probably need to crank up the z-index

@ryankeairns: I'm not able to reproduce this, or maybe I don't understand the issue. Let's sync up about it.


https://github.com/elastic/eui/pull/8771#issuecomment-3002058735

We know, in the end, the child will need to be rather large to contain a 'tool' like the Analyzer graph... how do you suppose we approach or enhance for this option? For example, we may do something like add a fill option for the child size (e.g. s, m, fill)

@ryankeairns: Fill is doable, which I've verified from accidentally creating that effect in mid-development :D. I think this is mostly an issue of using a clip-path style that makes this happen.

Also 'in the end', we will need to support custom/specific sizes for the main flyout... which means whatever the sizes provided, we will need to handle and adapt

Interesting! I missed this from docs that came out from planning this feature. Is this only the case for the main flyout?

So that we are all on the same page, should we handle these requirements in a subsequent PR(s)? And, if so, perhaps it is best to keep this new functionality undocumented until these (and potentially other) enhancements are made?

To me, this both feel like follow-up items, which seem within my technical range, and I'm happy to continue working in this repo to achieve whatever tasks we need. I agree the new functionality could be undocumented (not in the EUI docs site) until suitable progress has been made.

tsullivan avatar Jun 25 '25 21:06 tsullivan

Thanks for the replies.

One visual 'bug' In my initial review, I noticed there is a gradient that appears over the child footer:

Wow, great catch! Fixed in https://github.com/elastic/eui/commit/f02a3b93cc1a2c2e961d1da053b7aa364f44bf72

Confirmed, lgtm!

The close button is layering behind the child flyout panel on the small breakpoint; probably need to crank up the z-index

@ryankeairns: I'm not able to reproduce this, or maybe I don't understand the issue. Let's sync up about it.

Dug a little deeper. This seems to have to do with the max-inline-size for the child flyout.

On small viewports (i.e. under small breakpoint), the max inline size should be set to 90vw, same as we do with other flyouts. Right now, this is not the case so the child is a bit too wide resulting in the right edge going out of view (including the close button)

Before: without 90vw max

After: with 90vw max

ryankeairns avatar Jun 25 '25 22:06 ryankeairns

Regarding the follow-up work on supporting additional sizes:

  1. Adding support for a larger child flyout is a hard requirement given the size of tools (e.g. Analyzer; see image below). My proposal to start with supporting only s, m was to simplify our starting point... which you seem to have nailed :) Without a larger (e.g. custom child size) or fill option, Security won't be able to use the child flyout.
  2. Supporting custom parent flyout sizes is debatable, and I could see us moving ahead without it initially given the support for s, m . It appears, to me, that this would cover the use cases - for the parent slot - that we have seen to date. I could be wrong.

CleanShot 2025-06-25 at 15 36 01@2x

Note: they also support resizing which is another future enhancement consideration.


To clarify, I am not suggesting we hold this PR up. It tackles a healthy chunk of the task at hand, and it may serve other, less-complex use cases for parent-child flyouts, as-is.

My intent with this feedback is to clarify that there are additional requirement(s) needed before this can be adopted for Security use cases. Nothing surprising, just sharing context.

cc:/ @alexmarhaba

ryankeairns avatar Jun 25 '25 22:06 ryankeairns

Dug a little deeper. This seems to have to do with the max-inline-size for the child flyout.

@ryankeairns thanks for the detailed explanation! This has been fixed in f12ba501c

tsullivan avatar Jun 26 '25 01:06 tsullivan

Tested locally, the fix lgtm. Thanks!

ryankeairns avatar Jun 26 '25 15:06 ryankeairns

:green_heart: Build Succeeded

History

  • :green_heart: Build #638 succeeded 180089c67ae89240bf3f81d825a4ffe34d8ca477
  • :green_heart: Build #627 succeeded f12ba501c42e16ec074be1bfc719b6cc039ab93d
  • :green_heart: Build #626 succeeded 89d79d491e946099429effd096bb4c1817d35355
  • :green_heart: Build #625 succeeded ec1a72c02e3528e19b8a63682e17198a6561f19e
  • :green_heart: Build #624 succeeded f02a3b93cc1a2c2e961d1da053b7aa364f44bf72
  • :green_heart: Build #622 succeeded 42f8bf7dda650889c05aa95e7844cbce338dff19

cc @tsullivan

elasticmachine avatar Jun 27 '25 17:06 elasticmachine

:green_heart: Build Succeeded

History

  • :green_heart: Build #4277 succeeded 180089c67ae89240bf3f81d825a4ffe34d8ca477
  • :green_heart: Build #4269 succeeded f12ba501c42e16ec074be1bfc719b6cc039ab93d
  • :green_heart: Build #4268 succeeded 89d79d491e946099429effd096bb4c1817d35355
  • :green_heart: Build #4267 succeeded ec1a72c02e3528e19b8a63682e17198a6561f19e
  • :green_heart: Build #4266 succeeded f02a3b93cc1a2c2e961d1da053b7aa364f44bf72
  • :green_heart: Build #4264 succeeded 42f8bf7dda650889c05aa95e7844cbce338dff19

cc @tsullivan

elasticmachine avatar Jun 27 '25 18:06 elasticmachine