-
Notifications
You must be signed in to change notification settings - Fork 860
[EuiFlyout] add includeSelectorInFocusTrap
#8849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/eui/src/components/flyout/__snapshots__/flyout.test.tsx.snap
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| expect( | ||
| queryByText('You can still continue tabbing through', { exact: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the best way is to test the focusTrap props or functionality, so I will stick to this workaround that was already here.
tkajtoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good and work as expected. Thank you for your contribution!
|
@tkajtoch, thank you! Don't have access to merge 😅 pls help :) |
💚 Build SucceededHistory
|
mgadewoll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the code looks good to me but I haven't done a deep review/test as @tkajtoch already did one which is enough.
💚 Build Succeeded
History
|
`@elastic/eui`: `106.0.0` ⏩ `106.1.0` `@elastic/eslint-plugin-eui`: `2.1.0` ⏩ `2.2.1` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) New props and defaults have been added to `EuiFlyout`, including `includeSelectorInFocusTrap`, plus new global CSS variables for push mode offset control. Additionally, we introduced a small spacing fix for invalid `EuiDualRange` append component and added support for custom styles on `EuiTreeView` nodes via the css key. :warning: **React 16 is now deprecated:** EUI no longer tests against it or fixes issues specific to it. ## Changes - I removed forced click in: `x-pack/test/security_solution_cypress/cypress/tasks/rule_details.ts` - I removed `{enter}` in: `x-pack/test/security_solution_cypress/cypress/tasks/common/rule_actions.ts` (see #228517) ## Package updates ### `@elastic/eui` #### [`v106.1.0`](https://github.com/elastic/eui/releases/v106.1.0) - Added `--euiPushFlyoutOffsetInlineStart` and `--euiPushFlyoutOffsetInlineEnd` global CSS variables set by the `EuiFlyout` in `push` mode. ([#8872](elastic/eui#8872)) - Reduced the `min-width` for inputs in `EuiRange` and `EuiDualRange` ([#8866](elastic/eui#8866)) - Added `includeSelectorInFocusTrap` prop for `EuiFlyout` ([#8849](elastic/eui#8849)) - Added component defaults for `EuiFlyout` that include `includeSelectorInFocusTrap` and `includeFixedHeadersInFocusTrap` ([#8849](elastic/eui#8849)) **Bug fixes** - Fixed flaky manual return focus behavior on `EuiFlyout` by relying on `FocusTrap` returning focus instead ([#8878](elastic/eui#8878)) - Fixed `EuiFlyoutChild` closing its parent `EuiFlyout` on `Escape` keypress ([#8878](elastic/eui#8878)) - Fixed support for `css` key in items object passed to `EuiTreeView` ([#8864](elastic/eui#8864)) **Deprecations** - Deprecated support for React 16. Please update to a modern version of React or stay on EUI version 106.0.0 if you can't switch right now. ([#8868](elastic/eui#8868)) ### `@elastic/eslint-plugin-eui` #### [`v2.2.1`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.1) **Bug fixes** - Removed `no-css_color` entry which mapped to a duplicate `no-css-color` rule ([#8888](elastic/eui#8888)) #### [`v2.2.0`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.0) - Added new `prefer-eui-icon-tip` rule. ([#8877](elastic/eui#8877))
`@elastic/eui`: `106.0.0` ⏩ `106.1.0` `@elastic/eslint-plugin-eui`: `2.1.0` ⏩ `2.2.1` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) New props and defaults have been added to `EuiFlyout`, including `includeSelectorInFocusTrap`, plus new global CSS variables for push mode offset control. Additionally, we introduced a small spacing fix for invalid `EuiDualRange` append component and added support for custom styles on `EuiTreeView` nodes via the css key. :warning: **React 16 is now deprecated:** EUI no longer tests against it or fixes issues specific to it. ## Changes - I removed forced click in: `x-pack/test/security_solution_cypress/cypress/tasks/rule_details.ts` - I removed `{enter}` in: `x-pack/test/security_solution_cypress/cypress/tasks/common/rule_actions.ts` (see elastic#228517) ## Package updates ### `@elastic/eui` #### [`v106.1.0`](https://github.com/elastic/eui/releases/v106.1.0) - Added `--euiPushFlyoutOffsetInlineStart` and `--euiPushFlyoutOffsetInlineEnd` global CSS variables set by the `EuiFlyout` in `push` mode. ([elastic#8872](elastic/eui#8872)) - Reduced the `min-width` for inputs in `EuiRange` and `EuiDualRange` ([elastic#8866](elastic/eui#8866)) - Added `includeSelectorInFocusTrap` prop for `EuiFlyout` ([elastic#8849](elastic/eui#8849)) - Added component defaults for `EuiFlyout` that include `includeSelectorInFocusTrap` and `includeFixedHeadersInFocusTrap` ([elastic#8849](elastic/eui#8849)) **Bug fixes** - Fixed flaky manual return focus behavior on `EuiFlyout` by relying on `FocusTrap` returning focus instead ([elastic#8878](elastic/eui#8878)) - Fixed `EuiFlyoutChild` closing its parent `EuiFlyout` on `Escape` keypress ([elastic#8878](elastic/eui#8878)) - Fixed support for `css` key in items object passed to `EuiTreeView` ([elastic#8864](elastic/eui#8864)) **Deprecations** - Deprecated support for React 16. Please update to a modern version of React or stay on EUI version 106.0.0 if you can't switch right now. ([elastic#8868](elastic/eui#8868)) ### `@elastic/eslint-plugin-eui` #### [`v2.2.1`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.1) **Bug fixes** - Removed `no-css_color` entry which mapped to a duplicate `no-css-color` rule ([elastic#8888](elastic/eui#8888)) #### [`v2.2.0`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.0) - Added new `prefer-eui-icon-tip` rule. ([elastic#8877](elastic/eui#8877))
`@elastic/eui`: `106.0.0` ⏩ `106.1.0` `@elastic/eslint-plugin-eui`: `2.1.0` ⏩ `2.2.1` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) New props and defaults have been added to `EuiFlyout`, including `includeSelectorInFocusTrap`, plus new global CSS variables for push mode offset control. Additionally, we introduced a small spacing fix for invalid `EuiDualRange` append component and added support for custom styles on `EuiTreeView` nodes via the css key. :warning: **React 16 is now deprecated:** EUI no longer tests against it or fixes issues specific to it. ## Changes - I removed forced click in: `x-pack/test/security_solution_cypress/cypress/tasks/rule_details.ts` - I removed `{enter}` in: `x-pack/test/security_solution_cypress/cypress/tasks/common/rule_actions.ts` (see elastic#228517) ## Package updates ### `@elastic/eui` #### [`v106.1.0`](https://github.com/elastic/eui/releases/v106.1.0) - Added `--euiPushFlyoutOffsetInlineStart` and `--euiPushFlyoutOffsetInlineEnd` global CSS variables set by the `EuiFlyout` in `push` mode. ([elastic#8872](elastic/eui#8872)) - Reduced the `min-width` for inputs in `EuiRange` and `EuiDualRange` ([elastic#8866](elastic/eui#8866)) - Added `includeSelectorInFocusTrap` prop for `EuiFlyout` ([elastic#8849](elastic/eui#8849)) - Added component defaults for `EuiFlyout` that include `includeSelectorInFocusTrap` and `includeFixedHeadersInFocusTrap` ([elastic#8849](elastic/eui#8849)) **Bug fixes** - Fixed flaky manual return focus behavior on `EuiFlyout` by relying on `FocusTrap` returning focus instead ([elastic#8878](elastic/eui#8878)) - Fixed `EuiFlyoutChild` closing its parent `EuiFlyout` on `Escape` keypress ([elastic#8878](elastic/eui#8878)) - Fixed support for `css` key in items object passed to `EuiTreeView` ([elastic#8864](elastic/eui#8864)) **Deprecations** - Deprecated support for React 16. Please update to a modern version of React or stay on EUI version 106.0.0 if you can't switch right now. ([elastic#8868](elastic/eui#8868)) ### `@elastic/eslint-plugin-eui` #### [`v2.2.1`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.1) **Bug fixes** - Removed `no-css_color` entry which mapped to a duplicate `no-css-color` rule ([elastic#8888](elastic/eui#8888)) #### [`v2.2.0`](https://www.npmjs.com/package/@elastic/eslint-plugin-eui/v/2.2.0) - Added new `prefer-eui-icon-tip` rule. ([elastic#8877](elastic/eui#8877))
## Summary We introduced some EUI related hacks for flyouts for grid layout. After contributing some improvements to EUI we can clean the hacks up: - elastic/eui#8872 - this cleans up a hack to resize the push flyout using new exposed css variable - elastic/eui#8849 This allows us to specify which elements should be included in the flyout focus trap by default. Previously, it was the EUI fixed header, but in the grid layout, we don't have a fixed header. So, we need to tell EUI which elements to include: our header and sidenav. - Small unzyme --------- Co-authored-by: Tim Sullivan <[email protected]>
## Summary We introduced some EUI related hacks for flyouts for grid layout. After contributing some improvements to EUI we can clean the hacks up: - elastic/eui#8872 - this cleans up a hack to resize the push flyout using new exposed css variable - elastic/eui#8849 This allows us to specify which elements should be included in the flyout focus trap by default. Previously, it was the EUI fixed header, but in the grid layout, we don't have a fixed header. So, we need to tell EUI which elements to include: our header and sidenav. - Small unzyme --------- Co-authored-by: Tim Sullivan <[email protected]>
## Summary We introduced some EUI related hacks for flyouts for grid layout. After contributing some improvements to EUI we can clean the hacks up: - elastic/eui#8872 - this cleans up a hack to resize the push flyout using new exposed css variable - elastic/eui#8849 This allows us to specify which elements should be included in the flyout focus trap by default. Previously, it was the EUI fixed header, but in the grid layout, we don't have a fixed header. So, we need to tell EUI which elements to include: our header and sidenav. - Small unzyme --------- Co-authored-by: Tim Sullivan <[email protected]>
Summary
Part of #8820, this is an attempt to address
EuiFlyout's includeFixedHeadersInFocusTrapsub-issue where this prop won't longer work for Kibana in new grid layout since there won't be any fixed headers.As an alternative for Kibana I propose this more general
includeSelectorInFocusTrapthat can be configured for component defaults.This PR also refactors
includeFixedHeadersInFocusTrapcode to be built on top ofincludeSelectorInFocusTrapGeneral checklist
@defaultif default values are missing) and playground toggles