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.

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

allow esc to be defaultpreventable #158

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

keithamus
Copy link
Collaborator

Description

Browsers' native popover support allows for the Esc keypress to be preventable, which means that the popover won't close.

This is useful for capturing the Esc key behaviour from within a popover to do other things without closing the popover.

Currently the polyfill does not allow for this. This PR checks the flag to ensure default hasn't been prevented.

Steps to test/reproduce

Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).
I can follow up with steps/tests if we feel it necessary (time is short right now).

Show me

Provide screenshots/animated gifs/videos if necessary.

Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for popover-polyfill ready!

Name Link
🔨 Latest commit 74d2bda
🔍 Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/6568c7b263aa5d0008f23a96
😎 Deploy Preview https://deploy-preview-158--popover-polyfill.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jgerigmeyer jgerigmeyer merged commit 4aeb532 into main Nov 30, 2023
7 checks passed
@jgerigmeyer jgerigmeyer deleted the allow-esc-to-be-defaultpreventable branch November 30, 2023 17:36
@keithamus
Copy link
Collaborator Author

@jgerigmeyer do you think you could release this one as a patch please?

@jgerigmeyer
Copy link
Member

@jgerigmeyer do you think you could release this one as a patch please?

@keithamus Done!

@keithamus
Copy link
Collaborator Author

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants