[EuiFocusTrap] Allow `gapMode` and `crossFrame` props to be configured via `EuiProvider.componentDefaults` #6942

cee-chen · 2023-07-13T01:36:41Z

Summary

This PR contains a lot of cleanup (I realized EuiFocusTrap's props detection were essentially in shambles, which was making storybook testing annoying) so I strongly recommend following along by commit 🙏 The actual component default work is like 30% of the actual changes, haha.

Props table before (missing a ton of props, incorrect common props)

Props table after (still missing some default react-focus-on props, but significantly better)

QA

gh pr checkout 6942
yarn storybook
Go to http://localhost:6006/?path=/story/euifocustrap--eui-provider-component-defaults

`crossFrame` testing

In the controls pane, click on crossFrame and set it to true
In the story, click the Toggle focus trap button. Focus should now be on the input inside the EuiPanel
Try to click into/type into the Storybook "Find components" search box in the left sidebar outside the story
Confirm that focus is immediately trapped back into the input inside the EuiPanel
(Regression testing) Disable the focus trap, set the crossFrame control to false, re-toggle the focus trap, and then click to the outside left nav searchbox again. This time focus should not be trapped

`gapMode` testing

Setup

Revisit http://localhost:6006/?path=/story/euifocustrap--eui-provider-component-defaults to ensure no lingering state from the previous test
If on a Mac, go to your systems settings, search for "Scrollbar" and under "Show scroll bars", set this to "Always"

Test

In storybook, make the story frame as small as possible (either by zooming in or dragging the controls pane on the bottom up) until it overflows/scrolls vertically
In the controls pane, click on scrollLock and set it to true
In the controls pane, set gapMode to margin
In the story, click the Toggle focus trap button
Confirm that the scrollbar disappears but the width of it is preserved (EuiPanel should not change width)

Don't forget to restore your system scrollbar settings to your personal preference!

General checklist

Checked in Chrome, Safari, Edge, and Firefox
Checked for accessibility including keyboard-only and screenreader modes
Props have proper autodocs (using @default if default values are missing) and playground toggles
Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately

- it's kind of a dumpster fire right now with many props not being picked up so I'm wholesale rearranging many things - storybook props/controls also throws its own special brand of fun into this

…controls - we can remove the specified `crossFrame` arg now that it actually exists as a prop - default `returnFocus` to a boolean, since it can also take a function - add an `onDeactivation` state update (useful when testing `clickOutsideDisables=true`)

- these were already converted to Cypress tests, so there's no point / no need to keep them

+ add a `shouldRenderCustomStyles` to confirm that react-focus-on accepts className/css/style props

kibanamachine · 2023-07-13T02:01:38Z

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

tkajtoch · 2023-07-14T15:04:35Z

src/components/focus_trap/focus_trap.tsx

+  | 'noIsolation'
+  | 'returnFocus'
+> & {
+  // For some reason, Storybook doesn't register these props if they're Pick<>'d


I believe this is caused by too deep type inheritance that throws react-docgen-typescript off. I tried to fix it locally but had no luck so far. I know there's a fix for it though, we just need to spend some more time on it.

Anyway, I'm fine with keeping this as our solution in the meantime :)

Thanks for taking the time to test this Tomasz!

tkajtoch

Code looks great and the changes work as expected! 🚢

tkajtoch · 2023-07-14T15:17:43Z

src/components/focus_trap/focus_trap.test.tsx

-import React, { EventHandler } from 'react';
-import { render, mount } from 'enzyme';
+import React from 'react';
+import { render } from '../../test/rtl';


breehall

✅ This is a really nice clean up Cee (especially the components table)! Thank you for your detailed testing instructions as well. Everything worked according to your checklist, and this is all good to go!

…d via `EuiProvider.componentDefaults` (elastic#6942) * [cleanup] Fix EuiFocusTrap's props / props table - it's kind of a dumpster fire right now with many props not being picked up so I'm wholesale rearranging many things - storybook props/controls also throws its own special brand of fun into this * [cleanup] Update existing Storybook stories to account for new props controls - we can remove the specified `crossFrame` arg now that it actually exists as a prop - default `returnFocus` to a boolean, since it can also take a function - add an `onDeactivation` state update (useful when testing `clickOutsideDisables=true`) * [cleanup] Remove skipped focus trap Jest tests - these were already converted to Cypress tests, so there's no point / no need to keep them * [cleanup] Convert Jest tests to RTL + add a `shouldRenderCustomStyles` to confirm that react-focus-on accepts className/css/style props * Configure `EuiFocusTrap` to accept component defaults * Add Cypress test for `gapMode` * Add Storybook story for `crossFrame` & misc EuiProvider QA * changelog

…d via `EuiProvider.componentDefaults` (#6942) * [cleanup] Fix EuiFocusTrap's props / props table - it's kind of a dumpster fire right now with many props not being picked up so I'm wholesale rearranging many things - storybook props/controls also throws its own special brand of fun into this * [cleanup] Update existing Storybook stories to account for new props controls - we can remove the specified `crossFrame` arg now that it actually exists as a prop - default `returnFocus` to a boolean, since it can also take a function - add an `onDeactivation` state update (useful when testing `clickOutsideDisables=true`) * [cleanup] Remove skipped focus trap Jest tests - these were already converted to Cypress tests, so there's no point / no need to keep them * [cleanup] Convert Jest tests to RTL + add a `shouldRenderCustomStyles` to confirm that react-focus-on accepts className/css/style props * Configure `EuiFocusTrap` to accept component defaults * Add Cypress test for `gapMode` * Add Storybook story for `crossFrame` & misc EuiProvider QA * changelog

…d via `EuiProvider.componentDefaults` (elastic#6942) * [cleanup] Fix EuiFocusTrap's props / props table - it's kind of a dumpster fire right now with many props not being picked up so I'm wholesale rearranging many things - storybook props/controls also throws its own special brand of fun into this * [cleanup] Update existing Storybook stories to account for new props controls - we can remove the specified `crossFrame` arg now that it actually exists as a prop - default `returnFocus` to a boolean, since it can also take a function - add an `onDeactivation` state update (useful when testing `clickOutsideDisables=true`) * [cleanup] Remove skipped focus trap Jest tests - these were already converted to Cypress tests, so there's no point / no need to keep them * [cleanup] Convert Jest tests to RTL + add a `shouldRenderCustomStyles` to confirm that react-focus-on accepts className/css/style props * Configure `EuiFocusTrap` to accept component defaults * Add Cypress test for `gapMode` * Add Storybook story for `crossFrame` & misc EuiProvider QA * changelog

`v86.0.0`⏩`v87.1.0` ⚠️ The biggest set of type changes in this PR come from the breaking change that makes `pageSize` and `pageSizeOptions` now optional props for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and `EuiDataGrid.pagination`. This caused several other components that were cloning EUI's pagination type to start throwing type warnings about `pageSize` being optional. Where I came across these errors, I modified the extended types to require `pageSize`. These types and their usages may end up changing again in any case once the Shared UX team looks into #56406. --- ## [`87.1.0`](https://github.com/elastic/eui/tree/v87.1.0) - Updated the underlying library powering `EuiAutoSizer`. This primarily affects typing around the `disableHeight` and `disableWidth` props ([#6798](elastic/eui#6798)) - Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and `EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter typing ([#6798](elastic/eui#6798)) - Updated `EuiDatePickerRange` to support `compressed` display ([#7058](elastic/eui#7058)) - Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop ([#7061](elastic/eui#7061)) - Added a new `panelMinWidth` prop to `EuiInputPopover` ([#7071](elastic/eui#7071)) - Added a new `inputPopoverProps` prop for `EuiRange`s and `EuiDualRange`s with `showInput="inputWithPopover"` set ([#7082](elastic/eui#7082)) **Bug fixes** - Fixed `EuiToolTip` overriding instead of merging its `aria-describedby` tooltip ID with any existing `aria-describedby`s ([#7055](elastic/eui#7055)) - Fixed `EuiSuperDatePicker`'s `compressed` display ([#7058](elastic/eui#7058)) - Fixed `EuiAccordion` to remove tabbable children from sequential keyboard navigation when the accordion is closed ([#7064](elastic/eui#7064)) - Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs ([#7065](elastic/eui#7065)) **Accessibility** - Removed the default `dialog` role and `tabIndex` from push `EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual accessibility management. ([#7065](elastic/eui#7065)) ## [`87.0.0`](https://github.com/elastic/eui/tree/v87.0.0) - Added beta `componentDefaults` prop to `EuiProvider`, which will allow configuring certain default props globally. This list of components and defaults is still under consideration. ([#6923](elastic/eui#6923)) - `EuiPortal`'s `insert` prop can now be configured globally via `EuiProvider.componentDefaults` ([#6941](elastic/eui#6941)) - `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be configured globally via `EuiProvider.componentDefaults` ([#6942](elastic/eui#6942)) - `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and `showPerPageOptions` props can now be configured globally via `EuiProvider.componentDefaults` ([#6951](elastic/eui#6951)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow `pagination.pageSize` to be undefined. If undefined, `pageSize` defaults to `EuiTablePagination`'s `itemsPerPage` component default. ([#6993](elastic/eui#6993)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s `pagination.pageSizeOptions` will now fall back to `EuiTablePagination`'s `itemsPerPageOptions` component default. ([#6993](elastic/eui#6993)) - Updated `EuiHeaderLinks`'s `gutterSize` spacings ([#7005](elastic/eui#7005)) - Updated `EuiHeaderAlert`'s stacking styles ([#7005](elastic/eui#7005)) - Added `toolTipProps` to `EuiListGroupItem` that allows customizing item tooltips. ([#7018](elastic/eui#7018)) - Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers via `popoverContent` and `popoverProps` ([#7031](elastic/eui#7031)) - Improved the contrast ratio of disabled titles within `EuiSteps` and `EuiStepsHorizontal` to meet WCAG AA guidelines. ([#7032](elastic/eui#7032)) - Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a more clear visual indication of the current step ([#7048](elastic/eui#7048)) **Bug fixes** - Single uses of `<EuiHeaderSectionItem side="right" />` now align right as expected without needing a previous `side="left"` sibling. ([#7005](elastic/eui#7005)) - `EuiPageTemplate` now correctly displays `panelled={true}` ([#7044](elastic/eui#7044)) **Breaking changes** - `EuiTablePagination`'s default `itemsPerPage` is now `10` (was previously `50`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25, 50]` (was previously `[10, 20, 50, 100]`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - Removed `border` prop from `EuiHeaderSectionItem` (unused since Amsterdam theme) ([#7005](elastic/eui#7005)) - Removed `borders` object configuration from `EuiHeader.sections` ([#7005](elastic/eui#7005)) **CSS-in-JS conversions** - Converted `EuiHeaderAlert` to Emotion; Removed unused `.euiHeaderAlert__dismiss` CSS ([#7005](elastic/eui#7005)) - Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and `EuiHeaderSectionItemButton` to Emotion ([#7005](elastic/eui#7005)) - Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed `$euiHeaderLinksGutterSizes` Sass variables ([#7005](elastic/eui#7005)) - Removed `$euiHeaderBackgroundColor` Sass variable; use `$euiColorEmptyShade` instead ([#7005](elastic/eui#7005)) - Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead ([#7005](elastic/eui#7005)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>

cee-chen added 8 commits July 12, 2023 18:33

[cleanup] Fix EuiFocusTrap's props / props table

956fbf6

- it's kind of a dumpster fire right now with many props not being picked up so I'm wholesale rearranging many things - storybook props/controls also throws its own special brand of fun into this

[cleanup] Remove skipped focus trap Jest tests

2ccd048

- these were already converted to Cypress tests, so there's no point / no need to keep them

[cleanup] Convert Jest tests to RTL

d57d049

+ add a `shouldRenderCustomStyles` to confirm that react-focus-on accepts className/css/style props

Configure EuiFocusTrap to accept component defaults

43d2e97

Add Cypress test for gapMode

b8db223

Add Storybook story for crossFrame & misc EuiProvider QA

6447380

changelog

67d60f7

cee-chen marked this pull request as ready for review July 13, 2023 02:07

cee-chen requested a review from breehall July 13, 2023 02:08

tkajtoch reviewed Jul 14, 2023

View reviewed changes

tkajtoch approved these changes Jul 14, 2023

View reviewed changes

tkajtoch reviewed Jul 14, 2023

View reviewed changes

breehall approved these changes Jul 14, 2023

View reviewed changes

cee-chen merged commit d8e6a6e into elastic:feature/provider/component-defaults Jul 14, 2023

cee-chen deleted the provider/focus-trap branch July 14, 2023 19:48

cee-chen mentioned this pull request Aug 3, 2023

[EuiProvider] Add new componentDefaults configuration #6995

Merged

cee-chen mentioned this pull request Aug 15, 2023

Upgrade EUI to v87.1.0 elastic/kibana#163961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiFocusTrap] Allow `gapMode` and `crossFrame` props to be configured via `EuiProvider.componentDefaults` #6942

[EuiFocusTrap] Allow `gapMode` and `crossFrame` props to be configured via `EuiProvider.componentDefaults` #6942

cee-chen commented Jul 13, 2023 •

edited by tkajtoch

Loading

kibanamachine commented Jul 13, 2023

tkajtoch Jul 14, 2023

cee-chen Jul 14, 2023

tkajtoch left a comment

tkajtoch Jul 14, 2023

breehall left a comment

[EuiFocusTrap] Allow gapMode and crossFrame props to be configured via EuiProvider.componentDefaults #6942

[EuiFocusTrap] Allow gapMode and crossFrame props to be configured via EuiProvider.componentDefaults #6942

Conversation

cee-chen commented Jul 13, 2023 • edited by tkajtoch Loading

Summary

QA

crossFrame testing

gapMode testing

General checklist

kibanamachine commented Jul 13, 2023

tkajtoch Jul 14, 2023

Choose a reason for hiding this comment

cee-chen Jul 14, 2023

Choose a reason for hiding this comment

tkajtoch left a comment

Choose a reason for hiding this comment

tkajtoch Jul 14, 2023

Choose a reason for hiding this comment

breehall left a comment

Choose a reason for hiding this comment

[EuiFocusTrap] Allow `gapMode` and `crossFrame` props to be configured via `EuiProvider.componentDefaults` #6942

[EuiFocusTrap] Allow `gapMode` and `crossFrame` props to be configured via `EuiProvider.componentDefaults` #6942

cee-chen commented Jul 13, 2023 •

edited by tkajtoch

Loading

`crossFrame` testing

`gapMode` testing