-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add support for shadow DOM closed mode #75
Add support for shadow DOM closed mode #75
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for making a PR 👍 Is it possible to add a test case for this so that we don't regress on this in the future?
I added two tests (one for
|
e6c155a
to
b2ac4f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a bunch for adding tests 👍
Thanks for the review! What's the next step from here? I assume you'll handle the merge and the publish to npm? |
Hi @marvinhagemeister , what's the next step here. Can we get this out? |
Hi, any updates on this PR? |
Test CI still fails it seems |
@marvinhagemeister if you update this branch it may pass now. |
b2ac4f9
to
595903f
Compare
Nice, the tests are passing! |
Seems like this is not included in the latest version, when can we expect this to get released? |
Also need this change and wondering when it will be released! |
@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? |
@janbiasi seems we can change our package.json to grab from the repo directly { |
preactjs/preact-custom-element#75 added support for closed mode preactjs/preact-custom-element@e6bbe4f
preactjs/preact-custom-element#75 added support for closed mode preactjs/preact-custom-element@e6bbe4f
@HowieG Well yes, that works as a workaround at least 😄 |
@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. |
@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 :) |
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 #85 for this. cc @HowieG |
Fixes #74