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

Overlay: Attach escape handler to overlay container #1824

Merged
merged 20 commits into from
Jan 27, 2022
Merged

Overlay: Attach escape handler to overlay container #1824

merged 20 commits into from
Jan 27, 2022

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Jan 26, 2022 •
edited
Loading

Hitting Escape on Overlays does not bubble up anymore.

Fixes #1802

Screenshots

Explainer: (tiny update on video: stopPropagation is now part of Overlay, you don't have to pass it manually) https://www.loom.com/share/962646dc566a41329c5d660c932ba985

Merge checklist

  • Added/updated tests
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: c6294a7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@siddharthkp siddharthkp changed the title Escape handlers Overlay: Attach event handlers to overlay container Jan 26, 2022
@siddharthkp siddharthkp changed the title Overlay: Attach event handlers to overlay container Overlay: Attach escape handler to overlay container Jan 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 26, 2022 •
edited
Loading

size-limit report 📦

Path Size
dist/browser.esm.js 61.34 KB (+0.11% 🔺)
dist/browser.umd.js 61.71 KB (+0.1% 🔺)

@siddharthkp
Merge branch 'siddharth/nested-overlay-handlers' of github.com:primer…
66ba99d 
…/react into siddharth/nested-overlay-handlers
@siddharthkp siddharthkp self-assigned this Jan 26, 2022
@siddharthkp siddharthkp added accessibility patch release bug fixes, docs, housekeeping react labels Jan 26, 2022
@siddharthkp siddharthkp marked this pull request as ready for review January 26, 2022 13:48
@siddharthkp siddharthkp requested a review from a team January 26, 2022 13:48
rezrah
rezrah reviewed Jan 26, 2022
View reviewed changes
src/hooks/useOnEscapePress.ts
@@ -38,16 +38,24 @@ function handleEscape(event: KeyboardEvent) {
* @param callbackDependencies {React.DependencyList} The dependencies of the given
* `onEscape` callback for memoization. Omit this param if the callback is already
* memoized. See `React.useCallback` for more info on memoization.
*
* @param containerRef {React.RefObject<HTMLElement>} The overlay element to attach the
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ great idea

rezrah
rezrah approved these changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

🙌 amazing @siddharthkp - thanks for fixing this.

@rezrah
Copy link
Contributor

rezrah commented Jan 26, 2022

Out of curiosity, did you test the canary on memex and verify this works?

@siddharthkp
Copy link
Member Author

siddharthkp commented Jan 26, 2022 •
edited
Loading

Out of curiosity, did you test the canary on memex and verify this works?

Yes, but the solution isn't ideal. You would have to add event.stopPropagation to all the overlay components on the settings page. I feel like that should probably be the default when you are inside an Overlay and we can handle that on our end, making it easier for users 🤔

Update: done! and tested with memex and it works well!

We should should test the release branch with memex nicely for this one!

@siddharthkp siddharthkp merged commit 4eab65e into main Jan 27, 2022
@siddharthkp siddharthkp deleted the siddharth/nested-overlay-handlers branch January 27, 2022 10:07
@primer-css primer-css mentioned this pull request Jan 27, 2022
Merged
@siddharthkp siddharthkp mentioned this pull request Jan 27, 2022
Closed
@iansan5653 iansan5653 mentioned this pull request Feb 10, 2022
Closed
siddharthkp added a commit that referenced this pull request Feb 10, 2022
@siddharthkp
Revert "Overlay: Attach escape handler to overlay container (#1824)"
d71cde4 
This reverts commit 4eab65e.
@siddharthkp siddharthkp mentioned this pull request Feb 10, 2022
Merged
colebemis pushed a commit that referenced this pull request Feb 10, 2022
@siddharthkp
Revert "Overlay: Attach escape handler to overlay container (#1824)" (#…
3f40522 
…1856)

* Revert "Overlay: Attach escape handler to overlay container (#1824)"

This reverts commit 4eab65e.

* add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rezrah rezrah rezrah approved these changes

Assignees

@siddharthkp siddharthkp

Labels
accessibility patch release bug fixes, docs, housekeeping react
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overlay: Events bubbling up to the page can conflict with global shortcuts
2 participants
@siddharthkp @rezrah