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

[Bug] PigmentCSS. mixins are not skipped in shouldSkipGeneratingVar #44212

Closed
o-alexandrov opened this issue Oct 25, 2024 · 3 comments
Closed
Assignees
Labels
customization: theme Centered around the theming features package: pigment-css Specific to @pigment-css/* status: waiting for author Issue with insufficient information

Comments

@o-alexandrov
Copy link
Contributor

o-alexandrov commented Oct 25, 2024

Steps to reproduce

Link to live example: packages/mui-material/src/styles/createMixins.js

Current behavior

The file createMixins contains invalid CSS to place in a CSS variable.
PigmentCSS adds this invalid CSS as the value of the generated --mui-mixins... CSS variables.

Expected behavior

mixins is skipped in cssVariables.shouldSkipGeneratingVar by default

Context

I stumbled on this issue, when using a minifier on the CSS.
If you refer to createMixins.js you can see you have @media queries in the keys. It breaks the css variables names.

This is what the CSS minifier highlighted:

warnings when minifying css:
▲ [WARNING] Expected ":" [css-syntax-error]

    <stdin>:1:5100:
      1 │ ...eight:56px;--mui-mixins-toolbar-@media (min-width:0px)-@media (o...
        │                                    ^
        ╵                                    :


▲ [WARNING] Expected ":" [css-syntax-error]

    <stdin>:1:5191:
      1 │ ...eight:48px;--mui-mixins-toolbar-@media (min-width:600px)-minHeig...
        │                                    ^
        ╵                                    :

Your environment

npx @mui/envinfo
System:
    OS: macOS 15.0.1
  Binaries:
    Node: 22.10.0 - /opt/homebrew/bin/node
    npm: 10.9.0 - /opt/homebrew/bin/npm
    pnpm: 9.9.0 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 130.0.6723.70
    Edge: 112.0.1722.39
    Safari: 18.0.1
  npmPackages:
    @emotion/react:  11.13.3 
    @emotion/styled:  11.13.0 
    @mui/base:  5.0.0-beta.59 
    @mui/core-downloads-tracker:  6.1.4 
    @mui/icons-material:  6.1.4 
    @mui/lab:  6.0.0-beta.12 
    @mui/material:  6.1.4 
    @mui/material-pigment-css:  6.1.4 
    @mui/private-theming:  6.1.4 
    @mui/styled-engine:  6.1.4 
    @mui/system:  6.1.4 
    @mui/types:  7.2.18 
    @mui/utils:  6.1.4 
    @pigment-css/react:  0.0.24 
    @pigment-css/vite-plugin:  0.0.24 
    @types/react:  18.3.11 
    react:  18.3.1 
    react-dom:  18.3.1 
    typescript:  5.5.4 

Search keywords: pigmentcss, mixins, shouldSkipGeneratingVar

@o-alexandrov o-alexandrov added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 25, 2024
@o-alexandrov o-alexandrov changed the title [Bug] PigmentCSS. mixins should skipped in shouldSkipGeneratingVar [Bug] PigmentCSS. mixins are not skipped in shouldSkipGeneratingVar Oct 25, 2024
@zannager zannager added the package: pigment-css Specific to @pigment-css/* label Oct 25, 2024
@brijeshb42
Copy link
Contributor

@siriwatknp I think there might be an already existing solution for this. Do you have any idea ?

@siriwatknp
Copy link
Member

@o-alexandrov Can you provide the reproducible sandbox? The default shouldSkipGeneratingVar already includes the mixins https://github.com/mui/material-ui/blob/master/packages/mui-material/src/styles/shouldSkipGeneratingVar.ts#L4

@siriwatknp siriwatknp added status: waiting for author Issue with insufficient information customization: theme Centered around the theming features and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 3, 2024
Copy link

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features package: pigment-css Specific to @pigment-css/* status: waiting for author Issue with insufficient information
Projects
None yet
Development

No branches or pull requests

4 participants