Expand EuiFlyout usage to help EuiCollapsibleNav #4713

cchaos · 2021-04-14T21:19:40Z

This PR mainly pushes a bunch of custom props implemented for EuiCollapsibleNav back up to EuiFlyout so that EuiCollapsibleNav can just extend EuiFlyout and EuiFlyout gets more customization options.

👉 EuiFlyout new/adjusted props

`as`

/**
* Sets the HTML element for `EuiFlyout`
*/
as?: T = 'div';

This helps the EuiCollapsibleNav or any other consumption to pass nav as an element directly.

`role`

/**
* Defaults to `dialog` which is best for most cases of the flyout.
* Otherwise pass in your own, aria-role, or `none` to remove it and use the semantic `as` element instead
*/
role?: 'none' | HTMLAttributes['role'];

This also helps the EuiCollapsibleNav or any other consumption to change the aria-role type to fit their needs.

`size` now accepts any viable CSS `width` value

/**
* Defines the width of the panel.
* Pass a predefined size of `s | m | l`, or pass any number/string compatible with the CSS `width` attribute
*/
size?: _EuiFlyoutSize | CSSProperties['width'];

Allows for real customization of the actual rendered flyout width instead of relying on max-width or overrides.

`closeButtonProps`

/**
* Extends EuiButtonIconProps onto the close button
*/
closeButtonProps?: EuiButtonIconProps;

Good for extending/changing things like data-test-subj or adding a className.

`closeButtonPosition`

/**
* Position of close button.
* `inside`: Floating to just inside the flyout, always top right;
* `outside`: Floating just outside the flyout near the top (side dependent on `side`). Helpful when the close button may cover other interactable content.
*/
closeButtonPosition?: 'inside' | 'outside';

This was added to push up EuiCollapsibleNav's custom implementation of a solid close button that floats outside of the flyout, instead of within. Purposefully, there was no documentation added around this as we want to try to stay consistent for default EuiFlyout types.

`outsideClickCloses`

/**
* Forces this interaction on the mask overlay or body content.
* Defaults depend on `ownFocus` and `type` values
*/
outsideClickCloses?: boolean;

Also another request of the EuiCollapsibleNav is to allow a version where there's no overlay mask obscuring the content beneath, but we still want it to close automatically when starting to interact with the page content.

Video

Screen.Recording.2021-04-19.at.04.26.08.PM.mp4

`side`

/**
* Which side of the window to attach to.
* The `left` option should only be used for navigation.
*/
side?: 'left' | 'right';

This removed the duplication of the CSS styles via the euiFlyout() Sass mixin in EuiCollapsibleNav and just added a quick manipulation of the position CSS for the flyout itself. Again, purposefully left this prop out of the docs examples as we want the default to always be right.

`type`

/**
* How to display the the flyout in relation to the body content;
* `push` keeps it visible, pushing the `<body>` content via padding
*/
type?: 'overlay' | 'push';

This one might be controversial. But, we have custom implementations like this in Kibana that would be good to maintain at the EUI level. It helps EuiCollapsibleNav maintain the docked status and how the body accommodates for it with padding.

`pushMinBreakpoint`

/**
* Named breakpoint or pixel value for customizing the minimum window width to enable the `push` type
*/
pushMinBreakpoint?: EuiBreakpointSize | number;

A pushed flyout still positions itself as fixed, but adds padding to the document's body element to accomodate for the flyout's width. Because this squishes the page content, the flyout changes back to overlay at smaller window widths. You can adjust this minimum breakpoint with pushMinBreakpoint.

👉 EuiCollapsibleNav now extends EuiFlyout

The props list has been shortened and instead extends all the same props as EuiFlyout (with a few exceptions like type because it manages this with isDocked).

🔔 Breaking changes

EuiFlyout now defaults to `ownFocus = true`

This means that any default implementations of EuiFlyout with display the overlay mask. This is inherently good for accessibility and the proper way to display EuiFlyouts. The version without the overlay mask is the lesser common use or should be less common.

To compensate: If you need users to maintain the ability to interact with page contents while the flyout is open, set ownFocus={false}. Otherwise, the overlay mask is appropriate.

EuiFlyout gets wrapped in an EuiPortal when `ownFocus = true`

Because z-indexes are terrible and overlay masks are always in a portal, but the flyout itself was not, we were using z-indexes to position these layers. But this broke as soon as position: relative was added to any element wrapping the EuiFlyout. So now we just append the flyout to the document body within the overlay mask if the overlay mask exists (when ownFocus = true).

To compensate: There shouldn't need to be anything to do here unless there's a specific reason you need the flyout to exist in the nested dom exactly where it is placed in the code.

EuiCollapsibleNav now manages width via prop instead of CSS

Since the component extends EuiFlyout which accepts a number or string value via the size prop, the Sass variable $euiCollapsibleNavWidth has been removed in favor of this React prop. Though, the default width is still the same 320px.

To compensate: Delete any usages of $euiCollapsibleNavWidth and apply your custom width as the size prop.

EuiOverlayMask no longer uses z-index alone for determining position `below` header

The z-index now actually matches that of the EuiHeader, but since it's in a portal (appended to the dom) it will alway be "above" to ensure it sits above any non-fixed headers. So in order to make it appear below the header, it now uses the top value to offset it's position. This is added in the euiHeaderAffordForFixed() Sass mixin.

To compensate: As long as you are still using the euiHeaderAffordForFixed() Sass mixin for other offsets based on fixed headers, there's nothing to do. Otherwise, you will have to afford for this manually.

Other

Added a isWithinMinBreakpoint() breakpoint service to help find when the browser width is actually above the breakpoint passed.

Checklist

…tead of Sass

Also does a better job with mobile sizing and not just overtaking the whole screen all the time

…es` props

…‘outside’ but default in EuiCollabsibleNav

kibanamachine · 2021-04-14T21:42:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

kibanamachine · 2021-04-15T21:38:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

src/components/flyout/flyout.tsx

kibanamachine · 2021-04-19T21:16:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

src/components/collapsible_nav/collapsible_nav.tsx

kibanamachine · 2021-04-20T19:43:54Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

…ate/collapsible-nav

Update/collapsible nav - forwardRef

kibanamachine · 2021-04-30T20:41:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

…e-nav # Conflicts: # CHANGELOG.md # src/components/collapsible_nav/__snapshots__/collapsible_nav.test.tsx.snap # src/components/flyout/__snapshots__/flyout.test.tsx.snap

cchaos · 2021-05-04T15:31:19Z

Jenkins, test this

kibanamachine · 2021-05-04T15:52:37Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

myasonik

🚀

src-docs/src/views/flyout/flyout_example.js

kibanamachine · 2021-05-06T21:57:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

kibanamachine · 2021-05-07T00:12:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

chandlerprall · 2021-05-11T17:39:45Z

src/components/flyout/flyout.tsx

+   * Defaults to `dialog` which is best for most cases of the flyout.
+   * Otherwise pass in your own, aria-role, or `none` to remove it and use the semantic `as` element instead
+   */
+  role?: 'none' | string;


What do you think about dropping none and relying on role={undefined} to unset instead?

Yeah I had tried that before and was getting a11y errors, and I just tried again by just setting role?: string but passing role={undefined} seems to completely ignore the undefined and always keep the default 'dialog'. I did try with null, which works but that means just changing this to role?: null | string; but undefined still won't work.

Should I go with null?

Ack, that's right, because the default value is applied when a value is undefined. I think null is a bit cleaner than 'none', but I'm good either way

I'll update to null

src/components/flyout/flyout.tsx

elizabetdev

Tested in Chrome, Firefox, Safari and Edge. LGTM! 🎉

I just have a few suggestions.

src-docs/src/views/flyout/flyout_example.js

elizabetdev · 2021-05-11T18:03:51Z

src-docs/src/views/collapsible_nav/collapsible_nav_example.js

@@ -72,11 +65,14 @@ export const CollapsibleNavExample = {
      props: { EuiCollapsibleNav },
      demo: <CollapsibleNav />,
      snippet: `<EuiCollapsibleNav
+  ownFocus={false}


Is there any reason we want the snippet to have ownFocus={false}?

Ah yeah, so in Kibana we're going to use this setting on the global nav and went back and forth whether our docs should have it off by default too, then I think I forgot to update the snippet. Good catch.

src-docs/src/views/flyout/flyout_example.js

chandlerprall

Changes LGTM! Lots of goods things here

thompsongl

I will likely not have time to review this today, but I'm good merging with the current approvals in place.

…e-nav

chandlerprall

null role change LGTM

kibanamachine · 2021-05-11T20:03:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4713/

cchaos added 14 commits April 13, 2021 18:21

Fix when animation happens with docked navs

42664ce

WIP _ Terrible

3170831

Conditionally render children of EuiCollapsibleNavGroup

d492775

Fixing click capture on toggle button

41db315

[BREAKING] Width of EuiCollapsibleNav is now add via style prop ins…

44207ea

…tead of Sass

Add playground

8b04e68

[EuiFlyout] Extending size prop to accept width types

1ea7c37

Also does a better job with mobile sizing and not just overtaking the whole screen all the time

[EuiFlyout] Added side prop

f3a9db0

[EuiFlyout] added type=push, pushBreakpoint and `outsideClickClos…

2e4068b

…es` props

[EuiFlyout] Added as prop

3076cc1

[BREAKING] Update EuiCollapsibleNav to use EuiFlyout

3cfbcdc

Added closeButton and closeButtonPosition to EuiFlyout and using …

e8a4232

…‘outside’ but default in EuiCollabsibleNav

Fixing up EuiCallapsibleNav with docking of EuiFlyout

a55d9c6

Finalizing (?) the components

c800e26

cchaos added the breaking change label Apr 14, 2021

cchaos added 2 commits April 15, 2021 15:52

Updating docs

294c3aa

[Breaking] EuiFlyout defaults to ownFocus = true

95fa072

cchaos changed the title ~~[WIP] Expand EuiFlyout usage with push type and side prop.~~ [WIP] Expand EuiFlyout usage to help EuiCollapsibleNav Apr 19, 2021

Cleanup

5a5f33c

cchaos commented Apr 19, 2021

View reviewed changes

src/components/flyout/flyout.tsx Outdated Show resolved Hide resolved

cchaos commented Apr 20, 2021

View reviewed changes

src/components/collapsible_nav/collapsible_nav.tsx Outdated Show resolved Hide resolved

[EuiFlyout] Wrapping in EuiPortal when ownFocus=true

680a3bb

thompsongl and others added 4 commits April 21, 2021 12:53

forwardRef

1833e6c

collapsible type

5022e0d

Merge remote-tracking branch 'cchaos/update/collapsible-nav' into upd…

6440aa4

…ate/collapsible-nav

Merge pull request #49 from thompsongl/update/collapsible-nav

c6c5883

Update/collapsible nav - forwardRef

cchaos added 2 commits May 3, 2021 11:57

Changing EuiFlyout tabindex=0 to -1

fe1edb3

Merge remote-tracking branch 'upstream/master' into update/collapsibl…

ea7b30b

…e-nav # Conflicts: # CHANGELOG.md # src/components/collapsible_nav/__snapshots__/collapsible_nav.test.tsx.snap # src/components/flyout/__snapshots__/flyout.test.tsx.snap

myasonik mentioned this pull request May 4, 2021

[Discover] Paging through records with mouse or keyboard should update flyout contents elastic/kibana#99115

Closed

myasonik approved these changes May 6, 2021

View reviewed changes

src-docs/src/views/flyout/flyout_example.js Show resolved Hide resolved

src-docs/src/views/flyout/flyout_example.js Show resolved Hide resolved

Update a11y callout

daeaa4c

Merge branch 'master' into update/collapsible-nav

b2ac007

chandlerprall reviewed May 11, 2021

View reviewed changes

src/components/flyout/flyout.tsx Show resolved Hide resolved

elizabetdev approved these changes May 11, 2021

View reviewed changes

cchaos added 2 commits May 11, 2021 15:04

accomodate -> accommodate

3364337

No ownFocus

5ac621f

chandlerprall approved these changes May 11, 2021

View reviewed changes

Change role=‘none’ to role=null

f93898d

thompsongl reviewed May 11, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into update/collapsibl…

3273e25

…e-nav

chandlerprall approved these changes May 11, 2021

View reviewed changes

cchaos merged commit f44cf94 into elastic:master May 11, 2021

cchaos deleted the update/collapsible-nav branch May 11, 2021 21:34

This was referenced Jun 3, 2021

Upgrade EUI to v34.3.0 elastic/kibana#101334

Merged

[EuiFlyout] Add testenv mock #4858

Merged

cchaos mentioned this pull request Jun 9, 2021

[EuiCollapsibleNav] Right direction for Collapsible nav #4303

Closed

cchaos mentioned this pull request Jun 21, 2021

[EuiFlyout] Add a "push" instead of "overlay" option #2025

Closed

cchaos mentioned this pull request Jul 29, 2021

[EuiCollapsibleNav] Don't animate isOpen and isDocked on mount #4286

Closed

cee-chen mentioned this pull request Feb 2, 2023

[EuiFlyout] Include all fixed EuiHeaders in flyout focus traps + improve screen reader accessibility #6566

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand EuiFlyout usage to help EuiCollapsibleNav #4713

Expand EuiFlyout usage to help EuiCollapsibleNav #4713

cchaos commented Apr 14, 2021 •

edited

Loading

kibanamachine commented Apr 14, 2021

kibanamachine commented Apr 15, 2021

kibanamachine commented Apr 19, 2021

kibanamachine commented Apr 20, 2021

kibanamachine commented Apr 30, 2021

cchaos commented May 4, 2021

kibanamachine commented May 4, 2021

myasonik left a comment

kibanamachine commented May 6, 2021

kibanamachine commented May 7, 2021

chandlerprall May 11, 2021

cchaos May 11, 2021

chandlerprall May 11, 2021

cchaos May 11, 2021

elizabetdev left a comment

elizabetdev May 11, 2021

cchaos May 11, 2021

chandlerprall left a comment

thompsongl left a comment

chandlerprall left a comment

kibanamachine commented May 11, 2021

Expand EuiFlyout usage to help EuiCollapsibleNav #4713

Expand EuiFlyout usage to help EuiCollapsibleNav #4713

Conversation

cchaos commented Apr 14, 2021 • edited Loading

👉 EuiFlyout new/adjusted props

as

role

size now accepts any viable CSS width value

closeButtonProps

closeButtonPosition

outsideClickCloses

side

type

pushMinBreakpoint

👉 EuiCollapsibleNav now extends EuiFlyout

🔔 Breaking changes

EuiFlyout now defaults to ownFocus = true

EuiFlyout gets wrapped in an EuiPortal when ownFocus = true

EuiCollapsibleNav now manages width via prop instead of CSS

EuiOverlayMask no longer uses z-index alone for determining position below header

Other

Checklist

kibanamachine commented Apr 14, 2021

kibanamachine commented Apr 15, 2021

kibanamachine commented Apr 19, 2021

kibanamachine commented Apr 20, 2021

kibanamachine commented Apr 30, 2021

cchaos commented May 4, 2021

kibanamachine commented May 4, 2021

myasonik left a comment

Choose a reason for hiding this comment

kibanamachine commented May 6, 2021

kibanamachine commented May 7, 2021

chandlerprall May 11, 2021

Choose a reason for hiding this comment

cchaos May 11, 2021

Choose a reason for hiding this comment

chandlerprall May 11, 2021

Choose a reason for hiding this comment

cchaos May 11, 2021

Choose a reason for hiding this comment

elizabetdev left a comment

Choose a reason for hiding this comment

elizabetdev May 11, 2021

Choose a reason for hiding this comment

cchaos May 11, 2021

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

thompsongl left a comment

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented May 11, 2021

cchaos commented Apr 14, 2021 •

edited

Loading

`as`

`role`

`size` now accepts any viable CSS `width` value

`closeButtonProps`

`closeButtonPosition`

`outsideClickCloses`

`side`

`type`

`pushMinBreakpoint`

EuiFlyout now defaults to `ownFocus = true`

EuiFlyout gets wrapped in an EuiPortal when `ownFocus = true`

EuiOverlayMask no longer uses z-index alone for determining position `below` header