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 support for css @property #1092

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

z4o4z
Copy link

@z4o4z z4o4z commented May 16, 2023

This PR adds support for CSS @property feature.

  • createVar - creates a @property definition, returns property var format var(--propertyName)
  • createGlobalVar - same as createVar but with a predefined name
  • getVarName - accepts a property var format and returns just a property name var(--propertyName) -> --propertyName

example:

import { style, createVar, keyframes, getVarName, fallbackVar } from '@vanilla-extract/css';

const color = createVar();
const angle = createVar({
  syntax: '<angle>',
  inherits: false,
  initialValue: '0deg',
});

const angleKeyframes = keyframes({
  '100%': {
    [getVarName(angle)]: '360deg',
  },
});

export const root = style({
  background: 'pink',
  color: 'blue',
  padding: '16px',
  transition: `opacity .1s ease, color .1s ease`, // Testing autoprefixer
  backgroundImage: `linear-gradient(${angle}, rgba(153, 70, 198, 0.35) 0%, rgba(28, 56, 240, 0.46) 100%)`,
  animation: `${angleKeyframes} 7s infinite ease-in-out both`,


  ':hover': {
    opacity: 0.8,
    color: color,
  },
  
  vars: {
    [color]: '#fef',
    [angle]: fallbackVar(angle, '138deg'),
  }
});

@changeset-bot
Copy link

changeset-bot bot commented May 16, 2023

🦋 Changeset detected

Latest commit: 53ec657

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages
Name Type
@vanilla-extract/babel-plugin-debug-ids Minor
@vanilla-extract/css Minor
@vanilla-extract/integration Patch
@vanilla-extract/rollup-plugin Patch
@fixtures/features Patch
@fixtures/layers Patch
@fixtures/low-level Patch
@fixtures/next-app-router Patch
@fixtures/next-pages-router Patch
@fixtures/recipes Patch
@fixtures/sprinkles Patch
@fixtures/template-string-paths Patch
@fixtures/themed Patch
@fixtures/thirdparty Patch
@fixtures/unused-modules Patch
@fixtures/thirdparty-dep Patch
@fixtures/thirdparty-dep-dep Patch
vanilla-extract-example-remix Patch
vanilla-extract-example-webpack-react Patch
@vanilla-extract-private/tests Patch
@vanilla-extract/esbuild-plugin-next Patch
@vanilla-extract/esbuild-plugin Patch
@vanilla-extract/jest-transform Patch
@vanilla-extract/parcel-transformer Patch
@vanilla-extract/vite-plugin Patch
@vanilla-extract/webpack-plugin Patch
@vanilla-extract-private/test-helpers Patch
@vanilla-extract/next-plugin Patch

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

@markdalgleish
Copy link
Contributor

Thanks for the PR! This looks really great.

To me this is just an extension of our current createVar API. The fact that it's natively called @property definitely challenges our current naming, but I think for now VE needs to be consistent in calling them "vars". If we rename them to properties, we'd do this across the whole library in a future major version. I think it's okay that internals refer to it as "property" though since this future-proofs the integration package against API changes.

So I think we'd need to make the following changes for now:

  • Overload createVar so the API is createVar({ syntax: '<color>', inherits: false, initialValue: '#fff' }).
  • Rename createGlobalProperty to createGlobalVar.
  • Replace getPropertyName with getVarName (which already exists in our private package). This would need to be exposed from css and dynamic packages since you'd want this at both build time and run time.

@z4o4z
Copy link
Author

z4o4z commented May 31, 2023

Thanks for the PR! This looks really great.

To me this is just an extension of our current createVar API. The fact that it's natively called @property definitely challenges our current naming, but I think for now VE needs to be consistent in calling them "vars". If we rename them to properties, we'd do this across the whole library in a future major version. I think it's okay that internals refer to it as "property" though since this future-proofs the integration package against API changes.

So I think we'd need to make the following changes for now:

  • Overload createVar so the API is createVar({ syntax: '<color>', inherits: false, initialValue: '#fff' }).
  • Rename createGlobalProperty to createGlobalVar.
  • Replace getPropertyName with getVarName (which already exists in our private package). This would need to be exposed from css and dynamic packages since you'd want this at both build time and run time.

hey @markdalgleish, thanks for your feedback! Updated the code, pls let me know if I need to change something else

@z4o4z
Copy link
Author

z4o4z commented Jun 16, 2023

@markdalgleish thanks for cleaning this up! is there something I should address before merging?

@z4o4z
Copy link
Author

z4o4z commented Jul 19, 2023

@markdalgleish any updates here, i'm really waiting for this feature?

@erwinsmit
Copy link

@markdalgleish any updates here, i'm really waiting for this feature?

This would be a nice addition. Currently I can only use properties using globalStyle with a @ts-ignore. That feels a bit dirty

globalStyle(`@property --headerBackground1`, {
  // @ts-ignore
  syntax: `'<color>'`,
  initialValue: colorPrimitives.neutrals.whites.W100,
  inherits: false,
});

@8bit-echo
Copy link

I'd like to bump this and see it land! 🙏 This would be a great feature for our design system to use.

@mikaalnaik
Copy link

Need this as well, any update to this PRs future @markdalgleish?

@lukelarsen
Copy link

I am also needing this in our current library. Any update?

Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

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

Hey, I'm keen to help get this PR through the door. For me, there's a few things still missing.

Testing @property CSS output in a fixture

Your example in examples/next is somewhat complex, and isn't actually being tested anywhere. We snapshot screenshots and CSS output of each of the fixture packages, so maybe adding a simple property in the themed fixture would be enough.

Unit testing CSS output

This is done in transformCss.test.ts. You probably just need to add a single test case that uses an object with type: 'property'.

Docs

We tend to be a bit particular about docs, so that can be left to a separate PR. However, you're more than welcome try write them yourself if you want.

@askoufis
Copy link
Contributor

askoufis commented Sep 22, 2024

@z4o4z Hey, are you still interested in getting this PR over the line? If not, I may look at picking it up myself in the near future.

@z4o4z
Copy link
Author

z4o4z commented Sep 24, 2024

hey @askoufis, i've added tests and updated docs. let me know if that looks good for you :) Thanks for helping with the PR!

Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up again @z4o4z, it's most of the way there. For me the last missing bit is docs for createGlobalVar, which should be fairly straightforward. I've also made a comment about keyframes below.

Once the last bits are ready there's also some playwright snapshots that need updating.

site/docs/api/create-var.md Outdated Show resolved Hide resolved
@z4o4z
Copy link
Author

z4o4z commented Sep 28, 2024

@askoufis addressed the feedback + improved documentation. I found out that parcel doesn't support @property and all parcel-related playwright tests are failing now, not sure what to do with it...

@z4o4z
Copy link
Author

z4o4z commented Sep 28, 2024

@askoufis addressed the feedback + improved documentation. I found out that parcel doesn't support @property and all parcel-related playwright tests are failing now, not sure what to do with it...

looks like it's not the parcel issue itself, parcel uses lightningcss to build CSS, and lightningcss doesn't support @property keyword (playground) :(

Created the issue: parcel-bundler/lightningcss#820

hope it's not a blocker...

not a real issue

packages/css/src/vars.ts Outdated Show resolved Hide resolved
site/docs/api/create-var.md Outdated Show resolved Hide resolved
@z4o4z
Copy link
Author

z4o4z commented Sep 28, 2024

looks like it wasn't an issue in the parcel, apparently initial-value is required for non * syntax, so it was failing with not really useful error. So i've made initialValue required on type level for non * syntax.

Should be good to merge

@z4o4z z4o4z requested a review from askoufis September 28, 2024 18:35
Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

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

Thanks for the quick updates. Just some more minor comments from me.

Also realised there's no changesets for the features. We'll need one for createVar and createGlobalVar, as well as a separate one for adding vars support to keyframes. There's actually a few more changes that need changeesets. I'll add them.

site/docs/global-api/create-global-var.md Show resolved Hide resolved
site/docs/global-api/create-global-var.md Outdated Show resolved Hide resolved
packages/css/src/vars.ts Outdated Show resolved Hide resolved
packages/css/src/vars.ts Outdated Show resolved Hide resolved
packages/css/src/vars.ts Outdated Show resolved Hide resolved
@askoufis
Copy link
Contributor

@markdalgleish Would you mind giving this another review when you've got some time?

@askoufis
Copy link
Contributor

Looks like linting is also failing, specifically prettier.

@z4o4z
Copy link
Author

z4o4z commented Sep 29, 2024

@askoufis thanks for the feedback! addressed everything

@z4o4z z4o4z requested a review from askoufis September 29, 2024 11:04
@z4o4z
Copy link
Author

z4o4z commented Oct 4, 2024

hey @askoufis @markdalgleish any chance we can merge it soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants