search

oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
BSD 3-Clause "New" or "Revised" License
254 stars 14 forks source link

Add `isPolyfilled` method #193

Closed didoo closed 6 months ago

didoo commented 6 months ago

Description

This PR adds a isPolyfilled function to the public methods exposed by the polyfill. The functions tests if the Popover API has been polyfilled, and it can be used to execute conditional code depending on if the Popover API is polyfilled or is native.

Related Issue(s)

https://github.com/oddbird/popover-polyfill/issues/189

netlify[bot] commented 6 months ago

Deploy Preview for popover-polyfill ready!

Name Link
Latest commit 5ad1870a17b14137e684449c346e6495fbb6b309
Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/66044096eb9d290008bdd5d6
Deploy Preview https://deploy-preview-193--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.

didoo commented 6 months ago

@keithamus @jgerigmeyer I was looking at ways to do automatic testing of the function but from what I see with the current suite of tests it's not possible. Or am I missing something?

keithamus commented 6 months ago

Yeah we don't really have unit tests. Frankly I'm not especially concerned about tests for this amount of code. It'll get shaken out during integration.

didoo commented 6 months ago

@jgerigmeyer there was a linting error that I was looking to fix but the PR is already merged. Has it been already fixed (automatically maybe)? Is it OK to leave it as it is?

jgerigmeyer commented 6 months ago

@jgerigmeyer there was a linting error that I was looking to fix but the PR is already merged. Has it been already fixed (automatically maybe)? Is it OK to leave it as it is?

Yeah that's fine -- I merged it and then pushed a quick fix 👍

Thanks for your contribution!