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

Add support for shadow DOM closed mode #75

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

stevenle
Copy link
Contributor

@stevenle stevenle commented Sep 9, 2022

Fixes #74

Copy link
Member

@marvinhagemeister marvinhagemeister left a 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?

@stevenle
Copy link
Contributor Author

stevenle commented Sep 10, 2022

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.

Copy link
Member

@marvinhagemeister marvinhagemeister left a 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 👍

@stevenle stevenle closed this Sep 11, 2022
@stevenle stevenle reopened this Sep 11, 2022
@stevenle
Copy link
Contributor Author

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

@AGrabovajFitA
Copy link

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

@leifriksheim
Copy link

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
Copy link

MattCCC commented Jul 5, 2023

Test CI still fails it seems

@bhollis
Copy link
Contributor

bhollis commented Aug 15, 2023

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

@bhollis
Copy link
Contributor

bhollis commented Aug 16, 2023

Nice, the tests are passing!

@marvinhagemeister marvinhagemeister merged commit 5c889f0 into preactjs:master Aug 16, 2023
1 check passed
@janbiasi
Copy link
Contributor

janbiasi commented Oct 8, 2023

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

@HowieG
Copy link

HowieG commented Oct 12, 2023

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

@HowieG
Copy link

HowieG commented Oct 12, 2023

@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
Copy link

HowieG commented Oct 12, 2023

@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"
}
}

HowieG added a commit to HowieG/DefinitelyTyped that referenced this pull request Oct 12, 2023
HowieG added a commit to HowieG/DefinitelyTyped that referenced this pull request Oct 12, 2023
@janbiasi
Copy link
Contributor

janbiasi commented Oct 12, 2023

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
Copy link

HowieG commented Oct 12, 2023

@janbiasi yooo we did it!! #84 hahaha

@HowieG
Copy link

HowieG commented Oct 12, 2023

@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.

DefinitelyTyped/DefinitelyTyped@1709f05

@janbiasi
Copy link
Contributor

@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
Copy link
Member

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

@janbiasi
Copy link
Contributor

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

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.

Add support for {mode: 'closed'}
9 participants