mui / pigment-css

Pigment CSS is a zero-runtime CSS-in-JS library that extracts the colocated styles to their own CSS files at build time.
MIT License
828 stars 39 forks source link

[docs] Invalid CSS output w/ pseudo classes in theme.vars #7

Open o-alexandrov opened 7 months ago

o-alexandrov commented 7 months ago

Steps to reproduce

Steps:

  1. Add the following to theme.colorSchemes
theme.colorSchemes.dark.mixins.example = {
  "&:hover": {
    background: `rgba(0, 0, 0, 0.26)`,
  },
}
  1. Try to build; it will throw a warning [WARNING] Expected ":" [css-syntax-error] (it's actually an error; CSS is invalid & also won't be minified) during CSS minification, because the above ends up as --mixins-example-&:hover-background (notice the invalid & symbol in the variable name)

Your environment

npx @mui/envinfo ``` System: OS: macOS 14.4 Binaries: Node: 21.7.1 - /opt/homebrew/bin/node npm: 10.5.0 - /opt/homebrew/bin/npm pnpm: 8.15.5 - /opt/homebrew/bin/pnpm Browsers: Chrome: 123.0.6312.87 Edge: 112.0.1722.39 Safari: 17.4 npmPackages: @pigment-css/react: 0.0.4 @pigment-css/vite-plugin: 0.0.4 @emotion/react: 11.11.4 @emotion/styled: 11.11.5 @mui/private-theming: 5.15.14 @mui/styled-engine: 6.0.0-alpha.0 @mui/system: 6.0.0-alpha.0 @mui/types: 7.2.14 @mui/utils: 5.15.14 @types/react: 18.2.73 => 18.2.73 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: 5.4.3 => 5.4.3 ```

Search keywords: pigment theme vars

siriwatknp commented 7 months ago

This is expected by design. We should do better at documenting it.

The theme.colorSchemes should contain only tokens (to be converted to CSS variables). What you provided is a stylesheet that could not be converted to a CSS variable.

If you want to create a reusable styles, you can:

o-alexandrov commented 7 months ago

Importing mixins from colorSchemes directly as-is won’t work, because the background might be different for each colorScheme.

So either the design of Pigment should change to allow this case, or there should be a converter of a path to a token from colorSchemes into a variable name to be used in mixins

siriwatknp commented 7 months ago

Importing mixins from colorSchemes directly as-is won’t work, because the background might be different for each colorScheme.

So either the design of Pigment should change to allow this case, or there should be a converter of a path to a token from colorSchemes into a variable name to be used in mixins

Thanks for the feedback, before going into the solution, can you explain in more detail about your use case? Do you want to customize a background based on a different color scheme?

theme.vars contains fields that refer to CSS variables. It cannot contain stylesheets.

o-alexandrov commented 7 months ago

@siriwatknp Since you mentioned using mixins within colorSchemes is prohibited by design, please refer to the example below for the proposal on reusing css variables in mixins outside of colorSchemes.

const theme = extendTheme({
  colorSchemes: {
    dark: {
       // these are the colorScheme-dependent tokens you want to use in mixins
      palette: {
        overlay: `black`,
        primary: {
          main: `#fafafa`,
        }
      }
    }
  },
  // then you want to define a reusable stylesheet within mixins
  mixins: {
    example: {
      /**
       * Pigment should have a function to make it type-safe.
       *   - you could also infer the type automatically from the theme.colorSchemes[string] and allow dot notation reference w/o a function (ex. `getVar`)
       *
       * If a function is your preference, then Pigment (`getVar` below) also needs to take into account the `cssVarPrefix` bundler's option see: https://github.com/mui/material-ui/tree/master/packages/pigment-css-react#adding-a-prefix-to-css-variables
       * 
       * In the example below, the function is named as `getVar`
       *     the type of the parameter can be
       *        import type { Paths } from "type-fest" // see: https://github.com/sindresorhus/type-fest/blob/main/source/paths.d.ts
       *        type getVar = (key: Paths<PigmentTheme["colorSchemes"][keyof PigmentTheme["colorSchemes"]]>) => `var(${string})`
       * 
       * @example how it would look like in the code
       * backgroundColor: getVar("palette.overlay")
       * "&:hover": {
       *   backgroundColor: getVar("palette.primary.main")
       * }
       * 
       * Below, how it's currently must be done.
       * It is not type-safe, but it works.
       */
      backgroundColor: `var(--palette-overlay)`,
      "&:hover": {
        backgroundColor: `var(--palette-primary-main)`,
      }
    }
  }
})
brijeshb42 commented 7 months ago

@o-alexandrov Could you also add a code example on how you'd use these mixins in your css or styled definitions ?

o-alexandrov commented 7 months ago

@brijeshb42 in this case, it'd be:

export const classNameWithMixin = pigment.css(({ theme }) => ({
  ...theme.mixins.example,
  display: `flex`
}))