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

Make global styles available to all themes #34334

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Aug 26, 2021

Fixes #34328

This PR makes global styles available to all themes.

  1. Themes without theme.json and no experimental-link-color support either: the stylesheet contains the core presets.
  2. Themes without theme.json but experimental-link-color support: the stylesheet contains the core and theme presets.
  3. Themes with theme.json: the stylesheet contains the core and theme presets, plus the result of merging core & theme styles.

How to test

Activate TT1-blocks: a theme with theme.json support. Visit the front end. Check the content of global-styles-inline-css is:

  • Both core & theme presets (classes and variables).
  • The block gap styles (see).
  • The theme styles.

Activate the TwentyTwentyOne theme: a theme without theme.json but with experimental-link-color support. Visit the front end. Check the content of global-styles-inline-css is:

  • Both core & theme presets (classes and variables).

Activate the TwentyTwenty theme: a theme without theme.json and no experimental-link-color support. Visit the front end. Check the content of global-styles-inline-css is:

  • Core presets (classes & variables).

Follow-ups

Remove the styles included here from the block-library package.

They just differ on what data they have:

1. Without theme.json and no experimental-link-color support either:
   the stylesheet contains the core presets & styles.

2. Without theme.json but experimental-link-color support:
   the stylesheet contains the core and theme presets,
   plus the core styles if any.

3. With theme.json:
   the stylesheet contains the core and theme presets,
   plus the result of merging core & theme styles.
@oandregal oandregal self-assigned this Aug 26, 2021
@oandregal oandregal added [Status] In Progress Tracking issues with work in progress Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 26, 2021
@oandregal
Copy link
Member Author

oandregal commented Aug 26, 2021

@youknowriad I'm seeing this piece of CSS in addition to the presets output for TwentyTwentyOne and TwentyTwenty. I presume this doesn't have to be enqueued unless there's a theme.json file?

body {
  --wp--style--block-gap: 24px;
}
.wp-site-blocks > * + * {
  margin-top: var(--wp--style--block-gap);
  margin-bottom: 0;
}

@oandregal oandregal requested a review from youknowriad August 26, 2021 15:11
@oandregal
Copy link
Member Author

ok, removed the block gap styles for TwentyTwentyOne and TwentyTwenty.

@oandregal oandregal marked this pull request as ready for review August 26, 2021 15:46
@oandregal oandregal removed the [Status] In Progress Tracking issues with work in progress label Aug 26, 2021
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

The test cases work for me as described.

ok, removed the block gap styles for TwentyTwentyOne and TwentyTwenty.

There may be a harmless edge case here:

  1. Activate a theme with a theme.json
  2. Make a post/page and add a block that inherits layout
  3. Switch to a theme without a theme.json
  4. Gutenberg will still generate alignment styles for those containers, but --wp--style--block-gap will be undefined.

@oandregal
Copy link
Member Author

@jffng That seems to happen in the transition from a theme with theme.json to a theme without theme.json, so it happens with or without this PR. I'd think it's unrelated to this.

@mkkeck
Copy link

mkkeck commented Sep 2, 2021

Why? and how could this be disabled?

@oandregal
Copy link
Member Author

Why? and how could this be disabled?

Hi Michael, the issue this fixes #34328 explains the rationale. Note that this won't add any additional CSS that is not already shipped by other means (block-editor, block-library). We're simply moving that CSS from the packages to the global stylesheet.

@mkkeck
Copy link

mkkeck commented Sep 2, 2021

Okay, I see.

But in my frontend is added:

<style id='global-styles-inline-css' type='text/css'>
/* ... */
--wp--preset--color-- 
/* ... */
</style>

This is what I want to disable because it's useless for me and bloats my frontend output.

@oandregal
Copy link
Member Author

This is what I want to disable because it's useless for me and bloats my frontend output.

Hey, that's how the global stylesheet works. These are used across all the color classes (color, background, border) and theme authors reported them as useful as well. We don't have plans to remove them or to add any option to do so automatically.

@mkkeck
Copy link

mkkeck commented Sep 2, 2021

Okay, I see the point.

But is it not really possible, via hook, to remove not used presets like gradients, duotones, colors?

I'm a frontend developer, I want to reduce code for faster loading. So why should I ship things, my theme and frontend never use?

Yes it is usefull for theme authors, but not really for theme or frontend developers.

@oandregal
Copy link
Member Author

But is it not really possible, via hook, to remove not used presets like gradients, duotones, colors?

Hey Michael, if the theme you're using does not define any presets, no classes and CSS custom properties will be enqueued. Is that enough for you?

A different thing is that the presets defined by core will always be enqueued. The rationale for this can be discussed at #29568 It boils down to have stable colors available across themes for patterns. We also plan to expose them in the UI for users soon (see new color panel) #27473 (comment)

$type = 'all';
if ( ! WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() ) {
$type = 'presets';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this $type check be part of gutenberg_experimental_global_styles_get_stylesheet directly? Meaning if there's no theme.json just pint the presets otherwise print everything?

Copy link
Contributor

@youknowriad youknowriad Sep 2, 2021

Choose a reason for hiding this comment

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

Could be inside this function that way it works consistently regardless of the caller.

public function get_stylesheet( $type = 'all' ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 not sure how to do it. There're other calls below where, in the same code path, where want to build two different stylesheets (pseudocode):

$block_styles  = gutenberg_experimental_global_styles_get_stylesheet( $tree, 'block_styles' );
$css_variables = gutenberg_experimental_global_styles_get_stylesheet( $tree 'css_variables' );

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand it, themes without theme.json shouldn't return just the "presets" like always. It shouldn't be the responsibility of the caller to switch the "type" depending on the theme, the caller just asks for the stylesheet and returning the right styles should be internal logic.

@mkkeck
Copy link

mkkeck commented Sep 2, 2021

Hi again André,

yes I understand:

At the moment, it’s frustrating as a user that upon switching to a theme that overwrites the default color palette you lose access to the rainbow-color set that comes with core. The core palette aims to provide quick access to distinct hues reliably. A theme palette generally aims to define a restricted set of semantic colors for branding.

But such people could also add / merge the system colors via theme settings on their own (that I've done before).

... if the theme you're using does not define any presets, no classes and CSS custom properties will be enqueued

That's right but my theme uses presets, custom properties and - of course - lot of classes:
theme.json

Is that enough for you?

Not really. The wp-presets - specially the gradients - should not be merged, if theme has no gradients or set to false.
global-style-inline-css

But I found a solution (in hope not having other side effects):

add_action('wp_enqueue_scripts', function() {
  wp_dequeue_style('global-styles');
  // ... code for to enqueue my own 'global-inline-css'
});
// or as an alternative:
//remove_action('wp_enqueue_scripts', 'wp_enqueue_global_styles');

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This works well in my testing (as advertised).
Maybe we could abstract away the "type" check as suggested above.

@oandregal oandregal merged commit 482650a into trunk Sep 2, 2021
@oandregal oandregal deleted the add/global-styles-to-all-themes branch September 2, 2021 14:51
@github-actions github-actions bot added this to the Gutenberg 11.5 milestone Sep 2, 2021
@oandregal
Copy link
Member Author

@mkkeck Note that the core classes and properties may be used by patterns and will be exposed to users through the color panel well. I'd think it's fine to dequeue the global stylesheet and do your own enqueueing if you don't personally mind about that.

I'd recommend checking regularly in the Gutenberg release announces for changes in that area to make sure changes still work for you, though, as removing the core CSS is unexpected and not something we can cover for.

@oandregal
Copy link
Member Author

Follow-up to remove CSS from the base-styles and block-library packages at #34510

@getsource getsource added the [Type] Enhancement A suggestion for improvement. label Sep 8, 2021
@oandregal oandregal added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 13, 2021
@oandregal
Copy link
Member Author

oandregal commented Sep 13, 2021

Devnote

To review/clarify/check when time comes to publish it.

We've made some changes to the stylesheets enqueued by default such as:

:root {
  --wp--preset--{preset-type}--{preset-slug}: <core-value>;
  --wp--preset--color--pale-pink: #f78da7;
  --wp--preset--font-size--huge: 42px;
  // ...
}
  • The preset classes now use these custom properties, and they look like:
.has-<slug>-<preset-type> { ... }
.has-pale-pink-color { color: var(--wp--preset--color--pale-pink) !important; } 
.has-huge-font-size { color: var(--wp--preset--font-size--huge) !important; }

Themes that use the theme.json can set these values in that file. Themes that don't, can set the CSS Custom Property in their stylesheet with something like:

:root {
 --wp--preset--font-size--huge: <theme-value>;
}

@landwire
Copy link

Devnote

To review/clarify/check when time comes to publish it.

We've made some changes to the stylesheets enqueued by default such as:

  • The preset classes for colors & font sizes that were previously in the block-library are now part of the global stylesheet, which is loader later (but before the theme's).
  • The preset classes now look like this: .preset-class { css-property: css-value !important; }. This is, they gain an !important they didn't have before.
  • There's also a new ruleset containing CSS Custom Properties for the presets (body { --wp--preset--color--value: value; ... })

Can we please remove the "!important" flag within the preset classes? Or at least add a filter that enables us to do so? See #34575 (comment)

I really do not want to start using the !important flag all over my theme to override things. My frontend theme is also CMS agnostic, so it's a pain to code more and more exceptions for WordPress.

@mkkeck
Copy link

mkkeck commented Sep 19, 2021

To review/clarify/check when time comes to publish it.

We've made some changes to the stylesheets enqueued by default such as:

  • The preset classes for colors & font sizes that were previously in the block-library are now part of the global stylesheet, which is loader later (but before the theme's).
  • The preset classes now look like this: .preset-class { css-property: css-value !important; }. This is, they gain an !important they didn't have before.
  • There's also a new ruleset containing CSS Custom Properties for the presets (body { --wp--preset--color--value: value; ... })

More and more the Gutenberg project dictates theme and frontend developers how to use, write and code their themes to work with Gutenberg.

General, I like the Gutenberg Editor, but sometimes there's project philosophy, which is not acceptable:

  • Putting tons of new features, are knitted with hot needle, wich breaking current themes and plugins
  • New features rolled out to WordPress core, flagged as experimental, no opt-in or opt-out with no documentation
  • Eat or die mentality of some peoples here

That things have nothing to do with "CODE IS POETRY".

New features are really welcome, but these features should always have an opt-in or opt-out before rolling out to the WordPress core. All these features should also apply filters and actions, which enables patching.
Things which may break themes or plugins should be better tested and documented.

It is really bad to leave users, after minor upgrading their WordPress, with broken themes or plugins (and as result broken websites).

I must say, before FSE, there were less such issues.
Of course many users out there, mainly using a hosted WordPress on a SASS, require the FSE. But I know users, which disagree.

But some core developers here, I think, won't here them.

WordPress and Gutenberg should be tools to help end users to build pages and write content, not more and not less.
Styling pages and content should be done with themes only. I know, that users want a quick and ready to run solution, but for such cases there shiped themes like TwentyTwentyOne with the WordPress install package. Please patch these themes instead patching Gutenberg to overwriting themes handcrafted by theme developers.
Gutenberg should help to build great websites and themes and not sabotage the work of theme authors.

@oandregal
Copy link
Member Author

This introduced a bug that I'm fixing at #34955

@oandregal
Copy link
Member Author

I've updated a bit the devnote for this.

@oandregal
Copy link
Member Author

Prepared #35424 to enqueue preset styles in the editor as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Enhancement A suggestion for improvement.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make the global stylesheet available for all themes
6 participants