Global Styles: Try style system at site edit#20530
Global Styles: Try style system at site edit#20530
Conversation
oandregal
added
the
Global Styles
|
Size Change: +664 B (0%) Total Size: 886 kB
ℹ️ View Unchanged
|
|
I'm open to create a separate PR for the color-controls, if that helps ship this faster. Happy to merge it as part of this as well. |
oandregal
requested review from
Soean,
TimothyBJacobs,
ajitbohra,
chrisvanpatten,
talldan and
youknowriad
as code owners
|
@nosolosw Amazing work on this so far! Thank you for taking what I started and extending it! I'm comfortable closing my PR to continue with this one. After poking around a bit, these are some of the things I noticed (not your fault! Just observations) Infrastructure⭐️Need core values for
|
ItsJonQ
changed the title
|
Nice feedback @ItsJonQ ! I've got all ready for a second round. To address each point separately:
|
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
Hi @nosolosw thank you for the work on this PR 👍
The work on global styles is complex with multiple moving parts, so this PR is not simple to review.
I wonder if we could simplify things a big by separating the PR into multiple PR's.
-
A PR with the new color components. That's a totally separate work where we will need to deal with compatibility with current usages, the color palette, etc.
-
A PR with the basis for global styles but dealing only with a variable for example line-height.
-
A PR dealing with some additional variables and maybe the way to compute a variable from another.
What do you think, would this separation make sense?
lib/global-styles.php
Outdated
| 'line-height-heading' => gutenberg_experimental_global_styles_generate_line_height( $line_height, 0.8 ), | ||
| 'font-size-heading-1' => gutenberg_experimental_global_styles_generate_font_size( $font_size, $font_scale, 5 ), | ||
| 'font-size-heading-2' => gutenberg_experimental_global_styles_generate_font_size( $font_size, $font_scale, 4 ), | ||
| 'font-size-heading-3' => gutenberg_experimental_global_styles_generate_font_size( $font_size, $font_scale, 3 ), |
There was a problem hiding this comment.
Would we have a UI to change all these values, or the user will not be able to change them?
There was a problem hiding this comment.
The current UI for font-size is this #21030 It only contains base and scale, so the other sizes should adapt from there.
I was also a bit concerned about global styles being opinionated about a style computation (which is styles/theme territory in my view). I'll give it a shot at making global styles agnostic about style computation, and pushing those to the edges (CSS land).
There was a problem hiding this comment.
As a first step, I've pushed computations based on font base & font scale to CSS land. I believe we want to evolve the theme.json to be able to do more things, but I'd like to do that in a subsequent PR --- otherwise, this will get too big to review.
|
|
||
| // Make object reference immutable | ||
| // so it stays the same in the function setters. | ||
| let styles = {}; |
There was a problem hiding this comment.
Should we move this inside the hook? And use the hook useRef with a start value of {}?
There was a problem hiding this comment.
Also, the comment says "Make object reference immutable", but it's using let, not const.
| // so controls can modify the styles. | ||
| const { editEntityRecord } = useDispatch( 'core' ); | ||
|
|
||
| const setColor = ( newStyles ) => { |
There was a problem hiding this comment.
We are generating new setters on every render. Should we consider the usage of useCallback/useMemo?
oandregal
force-pushed
the
try/global-styles-site-edit-ui-global
branch
2 times, most recently
from
7a9511d to
d406c91
Compare
|
Just confirming, does this PR only handle typography and not colors at the moment? It also does not appear to pull from my experimental-theme.json at the moment.
|
Yeah, I've reduced scope based on feedback. This is now in a transitioning state while I apply other improvements, will report when it's ready for review/test. |
oandregal
force-pushed
the
try/global-styles-site-edit-ui-global
branch
5 times, most recently
from
f601a58 to
5516a52
Compare
lib/global-styles.php
Outdated
| */ | ||
| function gutenberg_experimental_global_styles_get_core() { | ||
| return gutenberg_experimental_global_styles_get_from_file( | ||
| dirname( dirname( __FILE__ ) ) . '/experimental-default-global-styles.json' |
There was a problem hiding this comment.
This was moved from the top-level to lib/global-styles as per this comment.
The block's CSS is already setting line-height and takes precedence from wp-admin styles, so we no longer need this override.
The block's CSS is already taken precedence from wp-admin styles, so we no longer need this.
oandregal
force-pushed
the
try/global-styles-site-edit-ui-global
branch
from
a1de624 to
413eadc
Compare
There was a problem hiding this comment.
Ah got it, this happens because the order of the fills is not relevant to the order in which they are rendered.
In that case, I suggest, defining a variable with the inspector component to avoid reporting its declaration:
const blockInspector = ( <ComplementaryArea
scope="core/edit-site"
complementaryAreaIdentifier="edit-site/block-inspector"
title={ __( 'Block Inspector' ) }
icon={ cog }
className="edit-site-complementary-area"
>
<InspectorSlot bubblesVirtually />
</ComplementaryArea>
)
| ); | ||
| function Sidebar() { | ||
| const hasGlobalStylesSupport = useSelect( ( select ) => { | ||
| return Object.keys( select( 'core/block-editor' ).getSettings() ).some( |
There was a problem hiding this comment.
Could we do return !! select( 'core/block-editor' ).getSettings(). __experimentalGlobalStylesUserEntityId;?
|
|
||
| // Make object reference immutable | ||
| // so it stays the same in the function setters. | ||
| let styles = {}; |
| } | ||
|
|
||
| :root h6 { | ||
| font-size: calc(0.5px * var(--wp--typography--font-base) * var(--wp--typography--font-scale)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
For everyone following this thread: I'm going to let this branch sit for a few days as it is while I look at other PRs (font-size attribute, targeted selectors). |
karmatosed
added
the
[Priority] High
|
Closing this PR as it seems it was superseded by #24250. |
9 participants
This is a fork and supersedes #20061 by @ItsJonQ It adds a Global Styles sidebar with typography controls in the Site Editor.
A few notes:
wp-gsclass in favor of CSS variables in the blocks. Following suit on what was done for colors Introduce a support key for support global style colors in blocks #21021 and line-height Paragraph: Add LineHeightControl + Attribute #20775 this PR adds the font-size & font-weight CSS variables to Paragraph and Heading blocks.Test instructions
Test in
edit-site:Test in
edit-post: