-
Notifications
You must be signed in to change notification settings - Fork 318
Add support for @starting-style
rules
#1566
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: c392d65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
Is this getting any attention? |
I would love to get this merged as soon as possible! Anyone who can review this PR? |
@lfantone Thanks for the PR! I have a couple of requests for this PR:
|
Thanks for the comment @askoufis, yeah you are right about splitting this into two different PR. I will convert this one to be the Regarding the name for this properties, do you think we should go with |
I think I manage to get the e2e test with Playwright but all the parcel cases fails with: {
origin: '@parcel/transformer-css',
message: 'Unknown at rule: @starting-style',
name: 'SyntaxError',
stack: undefined,
codeFrames: undefined
} Which makes me wonder if parcel has support for this new css rule or if I'm missing something in some config. |
It should, though parcel uses |
ec4bbd9
to
efc89b4
Compare
PR updated! There were two different parcel versions installed, one in the Let me know if there is anything else that should be addressed |
@lfantone It looks like there's still some anchor positioning code lying around. Additionally it looks like you've accidentally deleted the starting-styles changeset instead of the anchor positioning changeset. |
CSS anchor positioning
rules@starting-style
rules
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.
We're pretty close on this. A few tweaks and some unit tests and it should be ready to merge.
@@ -60,3 +60,11 @@ export const styleVariantsCompositionInSelector = styleVariants({ | |||
globalStyle(`body ${styleVariantsCompositionInSelector.variant}`, { | |||
fontSize: '24px', | |||
}); | |||
|
|||
// Style with starting-style |
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.
// Style with starting-style |
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.
Looks like the newer version of parcel is re-ordering and deduplicating CSS, resulting in these changes. They're not incorrect changes though, just calling them out.
'@vanilla-extract/css': minor | ||
--- | ||
|
||
Add support for `@starting-style` rules |
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.
Add support for `@starting-style` rules | |
`style`: Add support for `@starting-style` rules | |
**EXAMPLE USAGE**: | |
```ts | |
import { style } from '@vanilla-extact/css'; | |
export const styleWithStartingStyle = style({ | |
backgroundColor: 'black', | |
'@starting-style': { | |
backgroundColor: 'white', | |
}, | |
}); | |
``` |
@@ -52,12 +52,16 @@ export type MediaQueries<StyleType> = Query<'@media', StyleType>; | |||
export type FeatureQueries<StyleType> = Query<'@supports', StyleType>; | |||
export type ContainerQueries<StyleType> = Query<'@container', StyleType>; | |||
export type Layers<StyleType> = Query<'@layer', StyleType>; | |||
export type StartingStyleQueries<StyleType> = { |
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.
Thoughts on the StartingStyleQueries
name? While the Queries
suffix aligns with most of the other types, it doesn't really make sense since @starting-style
isn't a query like @media
or @container
. It's more similar to @layer
in that it declares something. Maybe it should just be called StartingStyle
?
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.
Sounds like a good suggestion!
// Check if there are any nested at-rule keys inside this block. | ||
// The presence of any key starting with '@' indicates nested queries, | ||
// which are not allowed for @starting-style. |
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.
Just flagging that this is actually valid CSS. @media
inside @starting-style
is valid (example), but it relies on your browser supporting CSS nesting.
Actually, looking at the CSS output of your implementation, it generates nested CSS (@starting-style
inside a selector). However, @starting-style
is baseline, and it's newer than CSS nesting, so all browsers that support @starting-style
support CSS nesting, so I think this is fine. However, I think for the sake of simplicity, disallowing further nested CSS within @starting-style
is the right move for now. We can always remove this restriction in the future if necessary.
this.transformStartingStyle( | ||
root, | ||
selectorRule!['@starting-style'], | ||
conditions, | ||
); |
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 transformation inside selectors
needs a unit test.
this.transformStartingStyle( | ||
root, | ||
supportsRule!['@starting-style'], | ||
conditions, | ||
); |
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 transformation inside @supports
needs a unit test.
if (root.type === 'local') { | ||
this.transformSimplePseudos(root, rules, conditions); | ||
this.transformSelectors(root, rules, conditions); | ||
} |
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 logic should have a unit test too.
This pull request adds complete support for CSS Anchor positioning.
The update includes implementing all necessary types for rules as well as incorporating the new queries "@starting-style" and "@position-try", allowing for a better experience when using CSS anchor positioning rules
Closes #1521
EDIT: This PR will add support only for
@starting-style
rule. CSS Anchor position will be supported in a separate PR.