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

Check slotted content for autofocus delegate #150

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

jpzwarte
Copy link
Contributor

@jpzwarte jpzwarte commented Nov 24, 2023

Fixes #149

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for popover-polyfill ready!

Name Link
🔨 Latest commit dda43c0
🔍 Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/6560926319eb98000811e46e
😎 Deploy Preview https://deploy-preview-150--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.

Copy link
Member

@jgerigmeyer jgerigmeyer 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 the contribution! Looks fine to me -- @keithamus what do you think?

Copy link
Collaborator

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

The change LGTM. A minor nit is that the focusDelegate function comes directly from the spec, but it has now diverged from the spec text quite significantly. We should probably add some commentary that this is no longer spec compliant (even though the current function is desirable). Perhaps a follow up can be made here?

@jpzwarte
Copy link
Contributor Author

The change LGTM. A minor nit is that the focusDelegate function comes directly from the spec, but it has now diverged from the spec text quite significantly. We should probably add some commentary that this is no longer spec compliant (even though the current function is desirable). Perhaps a follow up can be made here?

I haven't read the spec, but as far as this PR is concerned: in Chrome & Safari, a slotted [autofocus] element has the same behavior as this PR.

I assume you will merge the PR since I can't do it?

@keithamus
Copy link
Collaborator

I haven't read the spec, but as far as this PR is concerned: in Chrome & Safari, a slotted [autofocus] element has the same behavior as this PR.

Yes I agree, getting the behaviour right is important. The concern was around the readability of this polyfill as all methods are mapped 1-1 to the spec, as much as we can, and any deviations are called out.

I assume you will merge the PR since I can't do it?

@jgerigmeyer is probably better merging this and releasing; I do have permissions to merge but not to release.

@jgerigmeyer jgerigmeyer merged commit 62a7a0c into oddbird:main Nov 28, 2023
7 checks passed
@jpzwarte jpzwarte deleted the fix/slotted-autofocus branch November 28, 2023 15:08
@jgerigmeyer
Copy link
Member

Released in v0.3.3

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.

focusDelegate method doesn't handle custom elements where [autofocus] is in the light DOM (slotted)
3 participants