Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[EuiFlyout] Enable shards for scoping element clicks; Optionally delay close event #5860

Merged
merged 21 commits into from
May 3, 2022
Merged

[EuiFlyout] Enable shards for scoping element clicks; Optionally delay close event #5860

merged 21 commits into from
May 3, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented May 2, 2022 •
edited
Loading

Summary

#5810 changed the mechanism by which outside clicks are handled in EuiFlyout. The new behavior is more correct, but conflicts with specific cases where outside clicks were previously not truly considered "outside" despite happening on an external element. EuiCollapsibleNav implementing the Elastic pattern is the clearest case of this.

Two solutions to the problem:

  1. Enable the shards prop from react-focus-on to specify that certain elements should be considered as a part of the EuiFlyout. In the case of EuiCollapsibleNav that means the element passed to the button prop, which now automatically gets added to the shards array.

EuiFocusTrap already accepted the shards prop by way of ...rest.

  1. Conditionally delay the close function firing in EuiFocusTrap until mouseup. This give the proper button time to work and is unnoticeable for normal outside click events.

Before

Screen.Recording.2022-05-03.at.1.03.28.PM.mov

After

Screen.Recording.2022-05-03.at.1.03.51.PM.mov

Checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl thompsongl changed the title [EuiFlyout] Enable shards for scoping element clicks [EuiFlyout] Enable shards for scoping element clicks; Optionally delay close event May 3, 2022
@cee-chen
Copy link
Member

cee-chen commented May 3, 2022 •
edited
Loading

Is there any chance you can take a before/after screencap of the behavior or QA steps? Honestly I'm still not able to repro whatever behavior y'all were seeing on the Elastic docs, not sure if it's because I'm on Firefox.

edit: oh derp, it's clicking the toggle/hamburger button to close 🤪

@kibanamachine
Copy link

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

@thompsongl thompsongl marked this pull request as ready for review May 3, 2022 18:53
@thompsongl thompsongl mentioned this pull request May 3, 2022
Closed
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

cee-chen
cee-chen reviewed May 3, 2022
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more testing comments, hopefully my last! 🤞

src/components/focus_trap/focus_trap.spec.tsx Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.spec.tsx Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.spec.tsx Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.spec.tsx Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.spec.tsx Outdated Show resolved Hide resolved
src/components/flyout/flyout.spec.tsx Outdated Show resolved Hide resolved
@thompsongl thompsongl requested a review from cee-chen May 3, 2022 21:03
cee-chen
cee-chen approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for responding so quickly to my millions of nits! I have some super small prop documentation suggestions, but otherwise this definitely fixes the issue and looks good to me

src/components/flyout/flyout.tsx Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Thanks, @constancecchen; helpful as always!

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 58ab6ad into elastic:main May 3, 2022
@1Copenut 1Copenut mentioned this pull request May 4, 2022
Closed
@thompsongl thompsongl mentioned this pull request May 9, 2022
Merged
@breehall breehall mentioned this pull request May 16, 2022
Merged
@cee-chen cee-chen mentioned this pull request Jan 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thompsongl @kibanamachine @cee-chen