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.

Sign up for GitHub

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

Jump to bottom

Global Styles: Try style system at site edit #20530

Closed
wants to merge 21 commits into from
Closed

Global Styles: Try style system at site edit #20530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6bd655c
Create initial GlobalStyle component within SiteEdit
Feb 6, 2020
0aa67a0
Iterate on Global Styles for site-edit
oandregal Feb 21, 2020
b5e2d6f
Make the typography panel open by default
oandregal Mar 31, 2020
207236f
Load assets for wp-admin including the edit-post
oandregal Mar 31, 2020
6c92579
Fix rebase odditiy
oandregal Mar 31, 2020
c30ce34
Render the InspectorControl after GlobalStyles
oandregal Mar 31, 2020
88dcd4e
Make strings translatable
oandregal Mar 31, 2020
b9a50e5
Add comments
oandregal Mar 31, 2020
b317ba8
Pass class to complementaryareas
oandregal Mar 31, 2020
650947e
Remove check about edit-site
oandregal Mar 31, 2020
1a3f50b
Do not render GlobalStyles if theme has no support
oandregal Mar 31, 2020
a2dfea9
Fix rebase oddity
oandregal Mar 31, 2020
b49cd0d
Remove unnecesary code
oandregal Mar 31, 2020
9888d92
Remove unnecesary color override
oandregal Mar 31, 2020
1b97ad7
Move font-weight resets to interface package
oandregal Apr 1, 2020
96d9e9f
Move font-size to interface package
oandregal Apr 1, 2020
f1d75bb
line-height reset: adject and move to interface package
oandregal Apr 1, 2020
76b6e04
Fix h2 line-height
oandregal Apr 1, 2020
e1413a8
Fix color for paragraphs
oandregal Apr 1, 2020
8eb2bd9
Add fix for background-color
oandregal Apr 1, 2020
413eadc
Moved to complementary area
oandregal Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,12 @@
"markdown_source": "../packages/format-library/README.md",
"parent": "packages"
},
{
"title": "@wordpress/global-styles",
"slug": "packages-global-styles",
"markdown_source": "../packages/global-styles/README.md",
"parent": "packages"
},
{
"title": "@wordpress/hooks",
"slug": "packages-hooks",
Expand Down
7 changes: 0 additions & 7 deletions experimental-default-global-styles.json
View file
Open in desktop

This file was deleted.

94 changes: 18 additions & 76 deletions lib/global-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ function gutenberg_experimental_global_styles_get_user() {
*
* @param array $post_status_filter Filter CPT by post status.
* By default, only fetches published posts.
* @param bool $should_create_draft Whether a new draft should be created
* if no CPT was found. False by default.
* @param bool $should_create_cpt Whether a new draft should be created
* if no CPT was found. False by default.
* @return array Custom Post Type for the user's Global Styles.
*/
function gutenberg_experimental_global_styles_get_user_cpt( $post_status_filter = array( 'publish' ), $should_create_draft = false ) {
function gutenberg_experimental_global_styles_get_user_cpt( $post_status_filter = array( 'publish' ), $should_create_cpt = false ) {
$user_cpt = array();
$post_type_filter = 'wp_global_styles';
$post_name_filter = 'wp-global-styles-' . strtolower( wp_get_theme()->get( 'TextDomain' ) );
Expand All @@ -111,11 +111,11 @@ function gutenberg_experimental_global_styles_get_user_cpt( $post_status_filter

if ( is_array( $recent_posts ) && ( count( $recent_posts ) === 1 ) ) {
$user_cpt = $recent_posts[0];
} elseif ( $should_create_draft ) {
} elseif ( $should_create_cpt ) {
$cpt_post_id = wp_insert_post(
array(
'post_content' => '{}',
'post_status' => 'draft',
'post_status' => 'publish',
'post_type' => $post_type_filter,
'post_name' => $post_name_filter,
),
Expand Down Expand Up @@ -148,7 +148,7 @@ function gutenberg_experimental_global_styles_get_user_cpt_id() {
*/
function gutenberg_experimental_global_styles_get_core() {
return gutenberg_experimental_global_styles_get_from_file(
dirname( dirname( __FILE__ ) ) . '/experimental-default-global-styles.json'
Copy link
Member Author

Choose a reason for hiding this comment

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

This was moved from the top-level to lib/global-styles as per this comment.

dirname( __FILE__ ) . '/global-styles/experimental-default-global-styles.json'
);
}

Expand All @@ -164,15 +164,21 @@ function gutenberg_experimental_global_styles_get_theme() {
}

/**
* Takes a Global Styles tree and returns a CSS rule
* Takes core, theme, and user preferences,
* builds a single global styles tree and returns a CSS rule
* that contains the corresponding CSS custom properties.
*
* @param array $global_styles Global Styles tree.
* @return string CSS rule.
*/
function gutenberg_experimental_global_styles_resolver( $global_styles ) {
function gutenberg_experimental_global_styles_resolver() {
$css_rule = '';

$global_styles = array_replace_recursive(
gutenberg_experimental_global_styles_get_core(),
gutenberg_experimental_global_styles_get_theme(),
gutenberg_experimental_global_styles_get_user()
);

$token = '--';
$prefix = '--wp' . $token;
$css_vars = gutenberg_experimental_global_styles_get_css_vars( $global_styles, $prefix, $token );
Expand All @@ -189,16 +195,6 @@ function gutenberg_experimental_global_styles_resolver( $global_styles ) {
return $css_rule;
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this check means that, if the FSE experiment is enabled, we'll load the CSS variables in all wp-admin, not only in edit-site pages.

I did this because the CSS vars are now inlined into the block's CSS, so if I load the edit-post I'd expect it to work the same as the edit-site (variables are loaded, so blocks look the same).

Another option would be to only load this in the edit-post & edit-site? I don't think that's necessary but happy to hear other thoughts. cc @jorgefilipecosta

Copy link
Member

Choose a reason for hiding this comment

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

It's worth noting that if #21102 is merged, the editor content will be iframed, so CSS variables outside of that iframe would have no effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

so CSS variables outside of that iframe would have no effect.

Something to be mindful of in the iframe PR. We have ways to communicate into and out of the iframe, so it should be possible to set things properly in both.

* Fetches the Global Styles for each level (core, theme, user)
* and enqueues the resulting CSS custom properties for the editor.
*/
function gutenberg_experimental_global_styles_enqueue_assets_editor() {
if ( gutenberg_experimental_global_styles_is_site_editor() ) {
gutenberg_experimental_global_styles_enqueue_assets();
}
}

/**
* Fetches the Global Styles for each level (core, theme, user)
* and enqueues the resulting CSS custom properties.
Expand All @@ -208,13 +204,7 @@ function gutenberg_experimental_global_styles_enqueue_assets() {
return;
}

$global_styles = array_merge(
gutenberg_experimental_global_styles_get_core(),
gutenberg_experimental_global_styles_get_theme(),
gutenberg_experimental_global_styles_get_user()
);

$inline_style = gutenberg_experimental_global_styles_resolver( $global_styles );
$inline_style = gutenberg_experimental_global_styles_resolver();
if ( empty( $inline_style ) ) {
return;
}
Expand All @@ -224,51 +214,6 @@ function gutenberg_experimental_global_styles_enqueue_assets() {
wp_enqueue_style( 'global-styles' );
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

We're no longer relying on the wp-gs class.

* Adds class wp-gs to the frontend body class.
*
* @param array $classes Existing body classes.
* @return array The filtered array of body classes.
*/
function gutenberg_experimental_global_styles_wp_gs_class_front_end( $classes ) {
if ( ! gutenberg_experimental_global_styles_has_theme_support() ) {
return $classes;
}

return array_merge( $classes, array( 'wp-gs' ) );
}

/**
* Adds class wp-gs to the block-editor body class.
*
* @param string $classes Existing body classes separated by space.
* @return string The filtered string of body classes.
*/
function gutenberg_experimental_global_styles_wp_gs_class_editor( $classes ) {
if (
! gutenberg_experimental_global_styles_has_theme_support() ||
! gutenberg_experimental_global_styles_is_site_editor()
) {
return $classes;
}

return $classes . ' wp-gs';
}

/**
* Whether the loaded page is the site editor.
*
* @return boolean Whether the loaded page is the site editor.
*/
function gutenberg_experimental_global_styles_is_site_editor() {
if ( ! function_exists( 'get_current_screen' ) ) {
return false;
}

$screen = get_current_screen();
return ! empty( $screen ) && gutenberg_is_edit_site_page( $screen->id );
}

/**
* Makes the base Global Styles (core, theme)
* and the ID of the CPT that stores the user's Global Styles
Expand All @@ -279,8 +224,7 @@ function gutenberg_experimental_global_styles_is_site_editor() {
*/
function gutenberg_experimental_global_styles_settings( $settings ) {
if (
! gutenberg_experimental_global_styles_has_theme_support() ||
! gutenberg_experimental_global_styles_is_site_editor()
! gutenberg_experimental_global_styles_has_theme_support()
) {
return $settings;
}
Expand Down Expand Up @@ -332,10 +276,8 @@ function gutenberg_experimental_global_styles_register_cpt() {

if ( gutenberg_is_experiment_enabled( 'gutenberg-full-site-editing' ) ) {
add_action( 'init', 'gutenberg_experimental_global_styles_register_cpt' );
add_filter( 'body_class', 'gutenberg_experimental_global_styles_wp_gs_class_front_end' );
add_filter( 'admin_body_class', 'gutenberg_experimental_global_styles_wp_gs_class_editor' );
add_filter( 'block_editor_settings', 'gutenberg_experimental_global_styles_settings' );
// enqueue_block_assets is not fired in edit-site, so we use separate back/front hooks instead.
add_action( 'wp_enqueue_scripts', 'gutenberg_experimental_global_styles_enqueue_assets' );
add_action( 'admin_enqueue_scripts', 'gutenberg_experimental_global_styles_enqueue_assets_editor' );
add_action( 'admin_enqueue_scripts', 'gutenberg_experimental_global_styles_enqueue_assets' );
}
14 changes: 14 additions & 0 deletions lib/global-styles/experimental-default-global-styles.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from the top-level. So posting here the old contents for comparison:

{
	"color": {
		"primary": "#52accc",
		"background": "white",
		"text": "black"
	}
}

Copy link
Member Author

@oandregal oandregal Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that I've removed the primary color as the color hook at #21021 doesn't implement it.

"color": {
"background": "white",
"text": "black"
},
"typography": {
"font-base": 16,
"font-scale": 1.2,
"font-weight-base": 400,
"font-weight-heading": 400,
"line-height-base": 1.5,
"line-height-heading": 1.5
}
}
11 changes: 11 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"@wordpress/element": "file:packages/element",
"@wordpress/escape-html": "file:packages/escape-html",
"@wordpress/format-library": "file:packages/format-library",
"@wordpress/global-styles": "file:packages/global-styles",
"@wordpress/hooks": "file:packages/hooks",
"@wordpress/html-entities": "file:packages/html-entities",
"@wordpress/i18n": "file:packages/i18n",
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ $blocks-button__height: 56px;
}
}

.wp-gs .wp-block-button__link:not(.has-background) {
Copy link
Member Author

Choose a reason for hiding this comment

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

See comment.

background-color: var(--wp--color--primary);
}

.is-style-squared .wp-block-button__link {
border-radius: 0;
}
Expand Down
27 changes: 26 additions & 1 deletion packages/block-library/src/heading/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
h6 {
background-color: var(--wp--color--background);
color: var(--wp--color--text);
line-height: var(--wp--typography--line-height);
line-height: var(--wp--typography--line-height, var(--wp--typography--line-height-heading));
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that, the line-height hook that is shown in the block inspector is using --wp--typography--line-height, which is added as an inline style. If not present, it'll fall back to the global variable. This may need iteration in a subsequent PR, we may want to consolidate everything to general variables (potential direction, although needs discussion).

font-weight: var(--wp--typography--font-weight-heading);
Copy link
Member

Choose a reason for hiding this comment

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

Should we just remove the "-heading" suffix given that we will probably not have block specific variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've commented on this at #20530 (comment) TLDR is that so far we don't have a way to do otherwise. I proposed a potential direction but it needs some adjustments before is ready.

This is a discussion that also needs to address what do we do with font-size (used at the custom styles/block level) VS font base/scale (used at the global styles level) I mentioned at #20530 (comment)

Those seem conversations we need to have before settling on anything, so I lean towards merging this and then iterate. How does it sound?

}
}

Expand All @@ -21,3 +22,27 @@ h6 {
padding: $block-bg-padding--v $block-bg-padding--h;
}
}

:root h1 {
Copy link
Member Author

Choose a reason for hiding this comment

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

In the global styles UI, we aim to expose base and scale, and not font size directly. At the block UI level, we're instead inlining font-size values directly (see related PR to make font size an implicit attribute of the block). This may need some consolidation in further PRs.

We also may do with some pre-generation of variables or/and with some improvements to the theme.json, as @jorgefilipecosta (suggested). I think we may want to explore that separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share a little clarification on this? Currently the styles here auto-generate heading sizes, and do so with the :root h1 {} specificity, which overrides all of the usual theme-provided h1 {} heading styles. Is that the intended approach? I'm a little unclear how themes are supposed to provide heading sizes at the moment.

Copy link
Contributor

@jasmussen jasmussen Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm so incredibly excited about these features to land, and some of my favorite people are working on this!

However, and especially seeing the GIF above, the "base font" and "base scale" is the only aspect that feels unintuitive to me. I understand that the idea is to provide a harmonious typescale, and to provide a single control to affect all headings in one go.

But my first question is "how do I adjust only the H2?"

Tying this to a type-scale only, feels like it might end up frustrating users. What if I want my H2 to be almost exactly as big as my H1, and my H3 and H4 to be much smaller? If I'm reading this interface correctly, that's not possible in a global sense. More often than not the designs I've worked with use heading sizes that don't fit a typescale at all, but are still appropriate for the design in question. Many times they even use different font-weights or even fonts at different levels.

Don't get me wrong, the type scale is impressive, and there are many reasons to use one. But to me the type scale feels like an option to offer, rather than being the default or only interface.

CC: @karmatosed

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few things at play here:

  • Design-wise, I don't think I can personally provide any more input than what it's being discussed at Global styles: base font and scale #21030 (comment) The current direction is that users & themes should set base & scale. Blocks & Themes should use those variables to set the font-size.

  • There is also the :root h* bit, I can comment more on this. We're experimenting with adding some CSS variables to blocks by default (more context in thread). Some examples of current merged changes are line-height for heading, color & background-color for paragraph, heading, and Media/Text. This PR expands on that approach and adds font-weight and font-size by default to paragraph & heading. The vision is that this allows themes to config the CSS variables via the theme.json without needing to use them directly unless they explicitly want to expand what core does or do things differently.

Copy link
Member

Choose a reason for hiding this comment

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

Lots of great conversation here. I wanted to pop in and first say nothing here is set. I think what was mocked up was a good first bet, let's maybe see where this needs iterating.

There are a few options:

  • Just a scale: this has limited flexibility, however it could use the body text size to build from.
  • Set a base: this feels a more flexible approach but it also has complications.

I can totally see maybe this is theme optional, maybe it all is for base font and type scale. I however, do see that a user could find this really magical being able to go up and down.

Regarding individual headings, this is where I agree there needs to be some considered refinement. We are considering elements in groups, over individually. One option which is debatable if v1 or beyond (I could go either way), could be to have an option for more granular control 'reveal'. A simple then list of each heading could do that for headings. I'm totally into exploring around this.

font-size: calc(1px * var(--wp--typography--font-base) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale));
}

:root h2 {
font-size: calc(1px * var(--wp--typography--font-base) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale));
}

:root h3 {
font-size: calc(1px * var(--wp--typography--font-base) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale));
}

:root h4 {
font-size: calc(1px * var(--wp--typography--font-base) * var(--wp--typography--font-scale) * var(--wp--typography--font-scale));
}

:root h5 {
font-size: calc(1px * var(--wp--typography--font-base) * var(--wp--typography--font-scale));
}

:root h6 {
font-size: calc(0.5px * var(--wp--typography--font-base) * var(--wp--typography--font-scale));
Copy link
Member

Choose a reason for hiding this comment

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

I think there is some probability that they will break many teams.
We are only allowing to use global styles if theme has some kind of support, but we are unconditionally adding the styles that use the global styles variables.
I think given that styles are unconditional the global styles should also be. So, at least themes have a way to fix things using theme.json if the result is not expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR adds font-size and font-weight to existing blocks that have already added CSS to target line-height and/or colors as CSS variables. Is there any reason we should treat font-size or font-weight differently?

Copy link
Member

Choose a reason for hiding this comment

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

For line height, and colors we use the variables but these variables can be defined by the user. E.g: the user can use the UI to change line-height, and colors.

Here we use the variables and don't allow any control over the variables in most themes.
I think we should uncondiotanily show the global styles UI because we unconditionally add the styles that use the variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what you mean, thanks for the clarification 👍

Here's my thinking:

  • I think we can land this PR without user-facing UI controls: UI controls at the block level are nice, but they don't fix the potential issues. Example: for colors, the vars are only used when the users choose a custom color but the CSS is always present so the theme still needs to account for when the user doesn't modify anything. However! I've already started adapting the UI controls to work with font-size, I just don't think that work should block this.

  • As to whether to always load the global styles mechanism. Your point is solid: if we already ship block's CSS with variables, we should allow themes to set them. I may be on board with that, let's discuss it separately at Try/enable global styles #21346

}
4 changes: 3 additions & 1 deletion packages/block-library/src/paragraph/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
:root p {
background-color: var(--wp--color--background);
color: var(--wp--color--text);
line-height: var(--wp--typography--line-height);
line-height: var(--wp--typography--line-height, var(--wp--typography--line-height-base));
font-size: calc(1px * var(--wp--typography--font-base));
font-weight: var(--wp--typography--font-weight-base);
}

.is-small-text {
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"@wordpress/data": "file:../data",
"@wordpress/editor": "file:../editor",
"@wordpress/element": "file:../element",
"@wordpress/global-styles": "file:../global-styles",
"@wordpress/hooks": "file:../hooks",
"@wordpress/i18n": "file:../i18n",
"@wordpress/icons": "file:../icons",
Expand Down
62 changes: 35 additions & 27 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
__unstableEditorStyles as EditorStyles,
} from '@wordpress/block-editor';
import { useViewportMatch } from '@wordpress/compose';
import { GlobalStylesProvider } from '@wordpress/global-styles';

/**
* Internal dependencies
Expand Down Expand Up @@ -64,34 +65,41 @@ function Editor( { settings: _settings } ) {
<>
<EditorStyles styles={ settings.styles } />
<FullscreenMode isActive={ isFullscreenActive } />
<SlotFillProvider>
<DropZoneProvider>
<EntityProvider kind="root" type="site">
<EntityProvider
kind="postType"
type={ settings.templateType }
id={ settings.templateId }
>
<Context.Provider value={ context }>
<FocusReturnProvider>
<EditorSkeleton
sidebar={ ! isMobile && <Sidebar /> }
header={ <Header /> }
content={
<>
<Notices />
<Popover.Slot name="block-toolbar" />
<BlockEditor />
</>
}
/>
<Popover.Slot />
</FocusReturnProvider>
</Context.Provider>
<GlobalStylesProvider
userEntityId={ settings.__experimentalGlobalStylesUserEntityId }
baseStyles={ settings.__experimentalGlobalStylesBase }
>
<SlotFillProvider>
<DropZoneProvider>
<EntityProvider kind="root" type="site">
<EntityProvider
kind="postType"
type={ settings.templateType }
id={ settings.templateId }
>
<Context.Provider value={ context }>
<FocusReturnProvider>
<EditorSkeleton
sidebar={
! isMobile && <Sidebar />
}
header={ <Header /> }
content={
<>
<Notices />
<Popover.Slot name="block-toolbar" />
<BlockEditor />
</>
}
/>
<Popover.Slot />
</FocusReturnProvider>
</Context.Provider>
</EntityProvider>
</EntityProvider>
</EntityProvider>
</DropZoneProvider>
</SlotFillProvider>
</DropZoneProvider>
</SlotFillProvider>
</GlobalStylesProvider>
</>
) : null;
}
Expand Down
Loading