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

#34334 Breaks styles in theme's styles.css #34575

Closed
mkkeck opened this issue Sep 6, 2021 · 10 comments
Closed

#34334 Breaks styles in theme's styles.css #34575

mkkeck opened this issue Sep 6, 2021 · 10 comments
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Feedback Issues that relate purely to feedback on a feature that isn't necessarily actionable

Comments

@mkkeck
Copy link

mkkeck commented Sep 6, 2021

Description

#34334 Breaks some styles in themes-name/styles.css.
For example responsive font sizes defined in theme's styles.css won't work.

Step-by-step reproduction instructions

For example I 've follow rules in my CSS file which enables responsive font sizes:

.has-large-font-size {
  font-size: calc(1.275rem + .3vw)
}
@media (min-width: 1200px) {
  .has-large-font-size {
    font-size:1.5rem
  }
}

But with the #34334 there's an inline style added, which merges WordPress default global styles with the settings from theme.json which set's:

body {
  /* ... */
  --wp--preset--font-size--small: .875rem;
  --wp--preset--font-size--normal: 1rem;
  --wp--preset--font-size--medium: 1.25rem;
  --wp--preset--font-size--large: 1.5rem;
  --wp--preset--font-size--huge: 2.25rem;
}
.has-large-font-size {
  font-size: var(--wp--preset--font-size--large) !important;
}

The !important flag is here the problem.

I think this could break much other themes, so please add a filter or hook, to modify or disable the global-styles.

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 5.8, default Gutenberg
  • Browsers: all
  • Not theme specific

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@Sandstromer
Copy link

I have queried the use of the !important rule a few times in various issues.

It is quite often completely unnecessary, and with every new release of Gutenberg there seems to be more and more instances of !important being used.

Sorry I don't know what the answer is long term, as I don't think Gutenberg is going to stop using it any time soon.

@landwire
Copy link

I have exactly the same problem and it obviously breaks all my websites. The use of !important with the font-sizes, destroys my carefully crafted responsive typo styles. I happily would like to use a theme.json file for other stuff, so maybe it would be possible to either remove the "!important" or add a filter in "class-wp-theme-json-gutenberg.php" that you can remove it with.

@landwire
Copy link

landwire commented Sep 19, 2021

Another option obviously would be adding the possibility to use responsive sizes like so:

                {
                    "slug": "normal",
                    "size": {
                        "mobile": 18,
                        "tablet": 16,
                        "desktop": 18,
                        "wide": 22
                        "extra-wide": 24
                    }
                    "name": "Normal"
                },
                {
                    "slug": "big",
                    "size": 32,
                    "name": "Big"
                }
            ]

Created CSS vars:
--wp--preset--font-size--normal: 18px;
@media screen and (min-width: 600px) { /* tablet is 600 /
--wp--preset--font-size--normal: 16px;
}
@media screen and (min-width: 980px) { /
desktop is 980 */
--wp--preset--font-size--normal: 18px;
}
etc.

Ideally the mobile, tablet, desktop, wide, extra-wide variables are also defined somewhere in the theme.json.

I would still have the !important rule removed however.

@annezazu annezazu added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Feedback Issues that relate purely to feedback on a feature that isn't necessarily actionable labels Sep 24, 2021
@mkkeck
Copy link
Author

mkkeck commented Sep 25, 2021

Another option obviously would be adding the possibility to use responsive sizes like so:

                {
                    "slug": "normal",
                    "size": {
                        "mobile": 18,
                        "tablet": 16,
                        "desktop": 18,
                        "wide": 22
                        "extra-wide": 24
                    }
                    "name": "Normal"
                },
                {
                    "slug": "big",
                    "size": 32,
                    "name": "Big"
                }
            ]

Created CSS vars:
--wp--preset--font-size--normal: 18px;
@media screen and (min-width: 600px) { /* tablet is 600 /
--wp--preset--font-size--normal: 16px;
}
@media screen and (min-width: 980px) { /
desktop is 980 */
--wp--preset--font-size--normal: 18px;
}
etc.

Ideally the mobile, tablet, desktop, wide, extra-wide variables are also defined somewhere in the theme.json.

I would still have the !important rule removed however.

That would be an alternative way.
Adding different breakpoint sizes (aka Bootstrap) instead of device names could be also available.

@cbirdsong
Copy link

This probably needs to be a separate issue, but I would much rather see use of responsive typography values in theme.json instead of getting into adding breakpoints. The example in the OP could be expressed in a single value as min(1.275rem + .3vw, 1.5rem) instead of relying on a media query.

However, the !important’s gotta go no matter what. It should only be deployed a last resort, not as part of basic theme functionality.

@CHEWX
Copy link

CHEWX commented Dec 1, 2021

Theme.json basically breaks responsive typography. This is essential?!

@mkkeck
Copy link
Author

mkkeck commented Dec 2, 2021

Theme.json basically breaks responsive typography. This is essential?!

YES! THIS IS ESSENTIAL!
Global styles should never break theme styles.

@cbirdsong
Copy link

cbirdsong commented Jan 11, 2022

Bumping this issue up – this behavior breaks themes I’ve written when testing with 5.9 RC1.

We use responsive rem-based typography, and always treated the pixel value registered via add_theme_support as an approximation for use in the editor. It was never intended to be something to be output in the front end of the site, and the fact that you could not register rem units with the editor prior to theme.json meant it could not have been.

(Aside: It is extremely frustrating how cavalierly the Gutenberg team is treating compatibility with existing sites.)

@annezazu annezazu added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Feb 4, 2022
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Mar 11, 2022

@mkkeck, it seems like WordPress/wordpress-develop#2129 should have fixed part of this problem in 5.9.1, just leaving the question about some of the uses of ! important. There is another issue here which is also looking at that, what do you think about closing this issue, and making reference to it over there, so the discussion around this topic can be focused in one place?

@glendaviesnz
Copy link
Contributor

I am going to go ahead and close this in favour of #37590. Feel free to reopen it @mkkeck if you think there is extra detail in this issue that is not covered by that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Feedback Issues that relate purely to feedback on a feature that isn't necessarily actionable
Projects
None yet
Development

No branches or pull requests

7 participants