-
Notifications
You must be signed in to change notification settings - Fork 70
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: add support for @starting-style at-rule #1786
feat: add support for @starting-style at-rule #1786
Conversation
🦋 Changeset detectedLatest commit: 21413e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for compiled-css-in-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
context: mentioned in the PR description, but will repeat here:
The csstype dependency needed to be added so we could satisfy the Record<AtRules, boolean> type here.
- @starting-style was only added to the AtRules type in version 3.1.3 of csstype: https://github.com/frenic/csstype/blame/fb448e21733ac5cb52523d3b678fdbbe1f9b42f2/index.js.flow#L3573
- This project was previously not depending on csstype explicitly - an older version was being transitively used from @types/react: https://github.com/atlassian-labs/compiled/blob/master/yarn.lock#L4364
- This meant I also had to add @scope, as it was also added in version 3.1.3
- Alternatively, we could just locally extend the type with Record<AtRules | '@starting-style', boolean>. Let me know what you think 🙏
expect(actual).toIncludeMultiple([ | ||
'._syaz5scu{color:red}', | ||
// TODO: work out why this is failing. The below line is actually being output as '@starting-style color: blue{}' | ||
'@starting-style {._syaz5scu{color:blue}}', |
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.
question: this assertion isn't working out as I'd expect... Any ideas what I'm doing wrong? Is the input style syntax not valid?
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.
Interestingly, if I change the input to nest another property under @starting-style
, e.g.:
const styles = cssMap({
success: {
color: 'red',
'@starting-style': {
div: {
color: 'blue',
}
},
},
});
It outputs this:
@starting-style div{._1gzq13q2{color:blue}}
Which makes me think the plugin is doing something wrong? It should be this:
@starting-style { div{._1gzq13q2{color:blue}} }
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.
To sanity check, I tried @font-face
, which is an at-rule with a similar syntax to @starting-style
:
const styles = cssMap({
success: {
color: 'red',
'@font-face': {
fontFamily: 'Trickster',
},
},
});
This outputs:
@font-face font-family: Trickster
Which seems like the same problem.
The @media
test seems to work around this through nesting:
const styles = cssMap({
success: {
color: 'red',
'@media': {
'screen and (min-width: 500px)': {
color: 'blue',
},
},
},
});
Outputs:
@media screen and (min-width:500px){._1qhm13q2{color:blue}}
Which is valid syntax for @media
... But @starting-style
(and @font-face
) don't accept args, so doesn't make sense here.
@@ -20,7 +20,9 @@ const atRules: Record<AtRules, boolean> = { | |||
'@namespace': true, | |||
'@page': true, | |||
'@property': true, | |||
'@scope': true, |
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.
context: @scope
was also added as it is a member of the AtRules
type in the updated version of csstype
. Needed to satisfy the Record
type.
259ab67
to
21413e5
Compare
What is this change?
Adding support for the @starting-style at-rule.
Why are we making this change?
So it can be used in animations! Compiled currently throws an exception as it isn't aware of the property.
How are we making this change?
Mostly just adding
starting-style
to some lists.The
csstype
dev dependency needed to be added so we could satisfy theRecord<AtRules, boolean>
type here.@starting-style
was only added to theAtRules
type in version 3.1.3 ofcsstype
: https://github.com/frenic/csstype/blame/fb448e21733ac5cb52523d3b678fdbbe1f9b42f2/index.js.flow#L3573csstype
explicitly - an older version was being transitively used from@types/react
: https://github.com/atlassian-labs/compiled/blob/master/yarn.lock#L4364@scope
, as it was also added in version 3.1.3Record<AtRules | '@starting-style', boolean>
. Let me know what you think 🙏PR checklist
Don't delete me!
I have...
website/
- N/ATODO:
csstype
dependency