-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: Shadow DOM #57
feat: Shadow DOM #57
Conversation
@devknoll I just realized you pushed a commit with some significant changes while I was working on this. I will take a look at the new iteration. |
Apologies for the churn -- I was hoping to get those changes in before you rebased 🙂 |
no problem at all, I welcome the feedback/interaction 😄 |
b7070d7
to
1fed59b
Compare
#53 was merged, so you should be able to rebase against main now. |
@devknoll if you are good with current impls, would you be ok with me rebasing and landing this on |
900b435
to
7b67497
Compare
@devknoll this is ready for final review/merge when you have a moment to do another pass. |
7b67497
to
80c7d0b
Compare
Very sorry for the delay, I've been swamped with other things. Will review this today or tomorrow! |
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.
if you are good with current impls, would you be ok with me rebasing and landing this on main and then handling any additional work for ::slotted() ::part() or :host() in a separate PR?
@Marshal27 I'm not too keen on landing partial feature support. That said, I think enabling shadow roots at all may be a good incremental step, so I think I would be willing to land this if we deferred the :host
and related changes (e.g. processCssRules
), so that this PR was focused on just allowing you to use containers inside a shadow root that were defined inside that shadow root. The processCssRules
change is going to need some further iteration anyway I think.
How does that sound?
I agree with you on not landing partial work, but, I also agree with your recommended approach to keep this focused on enabling containers in the |
80c7d0b
to
b3fb604
Compare
f1ef64c
to
4031f5d
Compare
@devknoll if you spot any additional items let me know and I will get them implemented... if not, it should be GTG. |
4031f5d
to
f84c7a8
Compare
Thanks for your work on this! Though, I'm wondering is this supposed to be with just a standard style element or only with Constructable Stylesheets? I'm trying it out on a project where our existing web component templates include a style element inside of them, and with this PR it didn't work. Though, once I switched it to use constructable stylesheets it worked like a champ. |
f84c7a8
to
adf970b
Compare
@gkatsev I appreciate you bringing that to my attention. The latest push should provide basic support for standard style elements; please give it a test drive and let me know how it goes. Please note: Attempting either of the following is not currently working for standard style elements, however, these two approaches are currently working for the constructed stylesheets.
or
|
Awesome, let me try it out! |
Seems to be working now without requiring us to adopt Constructable Stylesheets immediately. Thanks! |
Thank you for the PR and for the maintainers for maintaining this polyfill. I hope this gets pulled in soon. |
adf970b
to
02c0463
Compare
02c0463
to
3c24f13
Compare
@Marshal27 Thank you for all of your hard work on this PR. This is an important feature that many folks are passionate about, and one that I had hoped we'd be able to get in. Unfortunately, I have moved on to other projects and there isn't currently anyone working full-time on the polyfill right now. Without an owner who can help see this full feature through to completion, we don't feel very comfortable merging this PR (or any other new features) right this moment. I'm very sorry about that That said, I highly encourage you to continue development on your fork: there are clearly many folks that want this feature and you're best poised to provide it. For testing, the repository is configured to use BrowserStack. You should know that BrowserStack provides free sponsorship for OSS projects, and we encourage you to apply so that you can continue to test your changes against the official Web Platform Tests, which include tests for ShadowDOM support. My sincere apologies |
Posting this link here for posterity. https://github.com/Marshal27/shadow-container-query-polyfill |
Awesome, I was wondering whether you'd be willing to maintain a community fork @Marshal27. |
I will definitely do my best. Feel free to add any shadow dom specific issues you may run into over there and I will do what I can. |
@devknoll Created this PR based on your #53
impl containing the following:
CSSStyleSheet.replace
andCSSStyleSheet.replaceSync
.CSSStyleSheet.replace
andCSSStyleSheet.replaceSync
if prototype function is defined.pending:
::slotted()
and::part()
:host()
(so far base level testing here of settingcontainer-type
seems to be working).