search

preactjs / preact-custom-element

Wrap your component up as a custom element
MIT License
360 stars 52 forks source link

Add support for shadow DOM closed mode #75

Closed stevenle closed 1 year ago

stevenle commented 2 years ago

Fixes #74

stevenle commented 2 years ago

I added two tests (one for {mode: 'open'} and one for {mode: 'closed'}. Would appreciate another look to make sure I'm following the correct patterns for this project.

(Note: I needed to update puppeteer in order for it to work on my Macbook with Apple Silicon. npm install wasn't working for me, so I used yarn and so the package-lock.json file wasn't updated.) Edit: Ignore this, I realized npm test:browsers worked without the puppeteer dependency.

stevenle commented 2 years ago

Thanks for the review! What's the next step from here? I assume you'll handle the merge and the publish to npm?

AGrabovajFitA commented 1 year ago

Hi @marvinhagemeister , what's the next step here. Can we get this out?

leifriksheim commented 1 year ago

Hi, any updates on this PR? I would love for this to get merged as we wouldn't have to keep our own fork of this project. 🙏

MattCCC commented 1 year ago

Test CI still fails it seems

bhollis commented 1 year ago

@marvinhagemeister if you update this branch it may pass now.

bhollis commented 1 year ago

Nice, the tests are passing!

janbiasi commented 1 year ago

Seems like this is not included in the latest version, when can we expect this to get released?

HowieG commented 1 year ago

Also need this change and wondering when it will be released!

HowieG commented 1 year ago

@janbiasi dang I just realized the last publish to npm was in 2020 😲 . Haven't encountered this before do we just clone the repo then?

HowieG commented 1 year ago

@janbiasi seems we can change our package.json to grab from the repo directly

{ "dependencies": { "preact-custom-element": "https://github.com/preactjs/preact-custom-element.git" } }

janbiasi commented 1 year ago

seems we can change our package.json to grab from the repo directly

@HowieG Well yes, that works as a workaround at least 😄 But I'm really questioning why this feature wasn't released via NPM. Maybe @marvinhagemeister can tell us more details about the official release.

HowieG commented 1 year ago

@janbiasi yooo we did it!! #84 hahaha

HowieG commented 1 year ago

@janbiasi FYI there wasn't a subsequent change to the types for this change. It's simple but need to go through DefinitelyTyped's kinda thorough PR process. I'm just ignoring for now.

https://github.com/DefinitelyTyped/DefinitelyTyped/commit/1709f05c255d8f1c30d115fb216722c39f6a7283

janbiasi commented 1 year ago

@HowieG Yea! 😎 Thanks for the support on this one. I think I should have time to go through the definitely typed PR process pretty soon but as you said, it also works without them - I'll keep you updated :)

rschristian commented 1 year ago

Alternatively, I think we're happy to add the types here. I don't think there's any real need to use DefinitelyTyped

janbiasi commented 1 year ago

Alternatively, I think we're happy to add the types here. I don't think there's any real need to use DefinitelyTyped

Thanks for the hint @rschristian, I just opened a PR https://github.com/preactjs/preact-custom-element/pull/85 for this.

cc @HowieG