-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support grouping rules like @media and @layer and fix shadow DOM support #103
Conversation
This reverts commit cfeee48.
@ghengeveld @JonathanKolnik 🙏 Could I get a review of this PR? I think it would open this addon up to a whole lot of new potential users that can't currently use it (like my team 😁). |
@m-akinc I'll take a good look at this and your other PRs tomorrow. You did a lot of great work here! |
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.
Wow, this is really great work! 👏
I left a few minor suggestions which you may want to look at. I also would've appreciated some new Storybook stories to showcase the scenarios this PR is addressing, so that we also have visual confirmation that it works as intended (and Chromatic will snapshot them to detect future regressions). Any chance you can add some?
Thanks a lot for all the effort! I couldn't have done all this myself.
Co-authored-by: Gert Hengeveld <info@ghengeveld.nl>
I added a story for a nested custom component and one for CSS at-rules. Let me know if you need anything else before merging. |
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.
LGTM. Nice work! Ready to merge?
Yes! 🚀 |
CI is failing to release, working on that... |
🚀 PR was released in |
Fixes #41 and #93
CSSGroupingRule
s like@layer
,@media
, etc.shadowHosts
because of the use of__pseudoStatesRewritten
. This would prevent them from getting classes applied byshadowHosts.forEach(updateShadowHost)
.:host(.foo) .control:hover
, it was incorrectly assuming any state selectors (e.g.:hover
) occurred inside the:host()
selector and generated a bogus selector like:host(.foo) .control.pseudo-hover-all
. Now handling scenarios like this by writing:host(.foo.pseudo-hover-all) .control
instead.pseudo-*-all
classes were being applied to a custom element (by theattachShadow
override) even if none of its stylesheets were rewritten (and therefore it was not added toshadowHosts
). This could result in those classes not being removed later.updateShadowHost
was only looking at classes on ancestors, so anypseudo-*
class applied directly to the shadow host because of a selector (i.e. inapplyParameter
) would get removed.updateShadowHost
would propagatepseudo-hover
, for example, from an ancestor to a shadow host. It should only be propagatingpseudo-*-all
.updateShadowHost
wouldn't cross shadow boundaries, so nested shadow DOM (i.e. a custom element in the shadow DOM of a custom element) wouldn't get the globalpseudo-*-all
class.rewriteStyleSheet
test cases