-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Collect user-defined CSS that can be used in @apply
#13349
base: next
Are you sure you want to change the base?
Conversation
dfd601f
to
e7836e7
Compare
e7836e7
to
0906a55
Compare
node.kind === 'rule' && | ||
node.selector[0] === '@' && | ||
node.selector.startsWith('@apply') | ||
) { |
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.
I'd flip the condition and do an early return here.
// processing, we have a circular dependency. | ||
if (candidate === rootAsCandidate) { | ||
throw new Error( | ||
`You cannot \`@apply\` the \`${candidate}\` utility here because it creates a circular dependency.`, |
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.
I think it'd be worth printing the selector of root
in the error message to help guide the user to the thing with the circular dependency.
i += 1 | ||
continue | ||
|
||
case ' ': // Descendat combinator |
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.
case ' ': // Descendat combinator | |
case ' ': // Descendant combinator |
// class that is a very simple selector, like `.foo` or `.bar`, but doesn't | ||
// contain any spaces, combinators, pseudo-selectors, pseudo-elements, or | ||
// attribute selectors. | ||
node.selector[0] === '.' && |
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.
I believe you don't need node.selector[0] === '.'
?
Because inside isSimpleClassSelector
, you perform this test with an early return:
if (selector[0] !== '.') return false
I would suggest to remove this line
node.selector[0] === '.' && |
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.
This is still required, if you comment this out a test should fail. This is basically for the scenario where you have .foo.bar
which contains 2 classes which is not allowed.
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.
Good catch 😉
node.selector.startsWith('@apply') | ||
) { | ||
let candidates = node.selector | ||
.slice(7 /* Ignore `@apply ` when parsing the selector */) |
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.
I really enjoy the comments in your code, you could have use
.slice('@apply '.length)
instead of
.slice(7 /* Ignore `@apply ` when parsing the selector */)
Off course the latest is slightly faster & shorter (if you omit the comment).
In the end, I prefer your version.
Longer with the comment but once you read it once, it feels right 🤓.
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.
Yep we focus on performance and re-allocating and re-computing the length is unnecessary work.
Good day, this amazing/magical feature seems to be forgotten, hopefully 🙏 this gets included in the next release, please @adamwathan |
This allows you to apply user-defined CSS inside of
@apply
. It works a tiny bit different compared to v3 because we only allow you to apply simple selectors. Only a single class can be applied such as a.foo {}
.If you want more complex selectors, then you can use nesting to enhance the definition of what
.foo
means exactly.Implementing it this way allows us to keep the codebase simple and keep expectations simple.
In v3 you can do crazy things such as:
And you can apply
@apply foo
or@apply bar
or@apply baz
, but that's not intuitive if you look at the final result.This PR also ensures that you can't apply classes that would result in circular dependencies.
What this PR is not:
This PR does not make custom classes "utilities" or "components". This means that you won't be able to do this:
Fixes: #13342