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 proper support for :not() #115

Merged
merged 1 commit into from
May 1, 2024

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Apr 15, 2024

The code avoided generating invalid CSS when a rule included :not(), but it did not provide correct support for the pseudo-*-all classes. For example, :not(:hover) would be transformed to .pseudo-hover-all :not(), but we would skip writing that rule, because of the invalid (i.e. empty) :not(). The correct rewriting of that rule is :not(.pseudo-hover-all *), i.e. "Not a descendant of an element with class .pseudo-hover-all". Similarly, for shadow DOM styles, we need special logic to turn the example rule into :not(:host(.pseudo-hover-all) *).

In general, the approach is:

  1. Replace :not(...<pseudo-state>...) with :not(.pseudo-<state>-all ...) (or :not(:host(.pseudo-<state>-all) ...) if for shadow DOM)
  2. Extract remaining pseudo-states and move them to an ancestor selector: ...<pseudo-state>... -> .pseudo-<state>-all ...

Additionally, the refactoring I did allowed the removal of special logic related to ::slotted() and :has().

@m-akinc
Copy link
Contributor Author

m-akinc commented Apr 15, 2024

@ghengeveld after this PR, I also have a change that adds support for :host-context(). After that, it should be time for a release. 🚀

@ghengeveld ghengeveld added patch Increment the patch version when merged skip-release Preserve the current version when merged labels Apr 15, 2024
@ghengeveld ghengeveld self-requested a review April 15, 2024 20:19
@m-akinc
Copy link
Contributor Author

m-akinc commented Apr 29, 2024

@ghengeveld seems like this stalled out. Anything I can do to help?

@ghengeveld
Copy link
Member

@m-akinc We had a company retreat last week so I was preoccupied. I'll take a look soon.

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work as always!

@ghengeveld ghengeveld merged commit 64a04cb into chromaui:main May 1, 2024
2 of 3 checks passed
@ghengeveld
Copy link
Member

Dang, seems like this broke a bunch of stories. Could you check it out @m-akinc? Perhaps address it in that follow-up PR?

@m-akinc
Copy link
Contributor Author

m-akinc commented May 2, 2024

Dang, seems like this broke a bunch of stories. Could you check it out @m-akinc? Perhaps address it in that follow-up PR?

@ghengeveld Sorry for that! Fix is up in #117. Another PR adding support for :host-context() will be following shortly. (Update: here it is: #118)

Copy link

github-actions bot commented May 6, 2024

🚀 PR was released in v3.1.0 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released This issue/pull request has been released. skip-release Preserve the current version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants