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

Global Styles: Try style system at site edit #20530

Open
wants to merge 21 commits into
base: master
from

Conversation

@nosolosw
Copy link
Member

nosolosw commented Feb 28, 2020

This is a fork and supersedes #20061 by @ItsJonQ It adds a Global Styles sidebar with typography controls in the Site Editor.

2020-03-30-global-styles-edit-site

A few notes:

  • The is not the final UI, we'll iterate in further PRs until we get to #21030
  • It focuses on the global typography first, leaving other things for subsequent PRs.
  • It adds the mechanics to read/update user & theme data via the Entities API.
  • It removes the use of the wp-gs class in favor of CSS variables in the blocks. Following suit on what was done for colors #21021 and line-height #20775 this PR adds the font-size & font-weight CSS variables to Paragraph and Heading blocks.
  • It loads the CSS variables in both edit-site & edit-post (see).
  • It leaves out the variable generation mechanism / variable calculation to further explore in a subsequent PR. See comment.
  • It uses the new interface package to add the global styles sidebar.

Test instructions

Test in edit-site:

  • Install and activate the Global Styles demo theme.
  • Enable the FSE Experiment under "Gutenberg > Experiments".
  • Go to "Gutenberg > Site Editor (beta)" and play with the Global Styles controls in the sidebar.
  • Save the changes to the Global Styles template via the "Update design" button in the editor.
  • Go to the front and see that your changes are applied and are the same as the editor.

Test in edit-post:

  • Install and activate the Global Styles demo theme.
  • Enable the FSE Experiment under "Gutenberg > Experiments".
  • Go to the post editor and create some content (paragraphs & headings). Make sure the values saved in the previous test are honored (inspect in the browser devtools, perhaps).
  • Edit the line-height of a paragraph or heading, and make sure it is updated.
@nosolosw nosolosw self-assigned this Feb 28, 2020
@nosolosw nosolosw requested a review from ItsJonQ Feb 28, 2020
@nosolosw nosolosw requested a review from jorgefilipecosta Feb 28, 2020
@nosolosw nosolosw added this to In progress in Global Styles: 🔴Red Phase via automation Feb 28, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Feb 28, 2020

Size Change: +664 B (0%)

Total Size: 886 kB

Filename Size Change
build/annotations/index.js 3.45 kB -1 B
build/block-directory/index.js 6.02 kB -1 B
build/block-editor/index.js 102 kB -9 B (0%)
build/block-library/index.js 110 kB -7 B (0%)
build/block-library/style-rtl.css 7.58 kB +78 B (1%)
build/block-library/style.css 7.59 kB +79 B (1%)
build/components/index.js 195 kB +3 B (0%)
build/core-data/index.js 10.7 kB +1 B
build/data-controls/index.js 1.03 kB +1 B
build/data/index.js 8.23 kB -3 B (0%)
build/dom-ready/index.js 568 B -1 B
build/dom/index.js 3.06 kB +1 B
build/edit-navigation/index.js 2.4 kB +1 B
build/edit-post/index.js 92.3 kB +2 B (0%)
build/edit-post/style-rtl.css 12.1 kB +38 B (0%)
build/edit-post/style.css 12.1 kB +40 B (0%)
build/edit-site/index.js 9.47 kB +384 B (4%)
build/edit-site/style-rtl.css 4.67 kB +49 B (1%)
build/edit-site/style.css 4.67 kB +50 B (1%)
build/edit-widgets/index.js 4.43 kB -1 B
build/editor/editor-styles-rtl.css 400 B -23 B (5%)
build/editor/editor-styles.css 402 B -24 B (5%)
build/editor/index.js 42.8 kB -1 B
build/element/index.js 4.45 kB +2 B (0%)
build/format-library/index.js 6.94 kB +1 B
build/hooks/index.js 1.93 kB +1 B
build/i18n/index.js 3.57 kB +1 B
build/keyboard-shortcuts/index.js 2.3 kB -1 B
build/notices/index.js 1.57 kB +1 B
build/nux/index.js 3 kB -3 B (0%)
build/plugins/index.js 2.54 kB +2 B (0%)
build/priority-queue/index.js 779 B -1 B
build/redux-routine/index.js 2.84 kB +3 B (0%)
build/server-side-render/index.js 2.54 kB +2 B (0%)
build/viewport/index.js 1.6 kB +1 B
build/warning/index.js 1.14 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 3.8 kB 0 B
build/autop/index.js 2.59 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/editor-rtl.css 7.21 kB 0 B
build/block-library/editor.css 7.21 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/edit-navigation/style-rtl.css 95 B 0 B
build/edit-navigation/style.css 95 B 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.74 kB 0 B
build/editor/style-rtl.css 3.47 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/global-styles/index.js 1.98 kB 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Feb 28, 2020

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.

@ItsJonQ

This comment has been minimized.

Copy link
Contributor

ItsJonQ commented Feb 28, 2020

@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 typography

The lack of these values is causing themes with an empty experimental-theme.json to break. In order for me to get this branch loading, I had to comment out some lines in the JS that expected values from styles.typography

⭐️experimental-theme.json from a theme directory is REPLACING core style values. Needs to EXTEND.

Values that are provided by a theme's experimental-theme.json are completely erasing the values provided by core.

Rendering

⭐️Remove computed values (line-height, font-sizes) for now (?) (in the JS logic)

Ideally, I hope that we're able to generated computed values from a single value. For example, fontSize -> all heading sizes. At the moment, I did this in the JS layer during updates. I feel like for this mechanism to be successful, it may have to be done server side (as well... maybe?).

It works okay if the computed values hook/function is able to execute - either on change, or maybe even on initial Edit Site load. However, if that doesn't happen, then no computed values happen.

⭐️Styles not being rendered (Gutenberg or Frontend) by default. Currently only works if specified by the theme.

No style updates occur if we switch to theme without the CSS variables hooked it, example. TwentyTwenty (assuming experimental-theme.json). Core blocks should be hooked up to these variables assuming global styles are activated with dedicated global styles CSS.

Theme: Styles

⭐️Lack of .wp-gs env scope is causing render issues in Gutenberg UI

An example of this is would be paragraphs when activating the (experimental) Global Styles theme:

Screen Shot 2020-02-28 at 1 45 58 PM

@ItsJonQ ItsJonQ mentioned this pull request Feb 28, 2020
1 of 3 tasks complete
@ItsJonQ ItsJonQ changed the title Try style system at site edit (only global styles) Global Styles: Try style system at site edit Feb 28, 2020
@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Mar 3, 2020

Nice feedback @ItsJonQ ! I've got all ready for a second round. To address each point separately:

  • Need core values for typography. Added. See experimental-default-global-styles.json.
  • Theme needs to extend core values. Fixed.
  • Computed values. I've added the same generation logic server-side for the moment. I'd like to explore this fully in a separate PR, including the possibility of perhaps creating a new endpoint to centralize all the generation logic in the server so mobile can reuse as well.
  • Theme needs to specify block styles with CSS variables. This is intentional so we can iterate faster by not messing with core styles at the moment. Note, however, that the demo playground theme that comes with this has separate stylesheets per block, so we can merge them back in core easily.
  • Lack of .wp-gs env scope. While the styles are external, we don't need this. However, what you spotted is true! The reason is that, unlike edit-post, edit-site doesn't wrap the styles using the editor-styles-wrapper. We need to fix that --- was thinking a different PR would be best.
Copy link
Member

jorgefilipecosta left a 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?

packages/components/src/color-control/index.js Outdated Show resolved Hide resolved
lib/global-styles.php Outdated Show resolved Hide resolved
'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 ),

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 12, 2020

Member

Would we have a UI to change all these values, or the user will not be able to change them?

This comment has been minimized.

Copy link
@nosolosw

nosolosw Mar 25, 2020

Author Member

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).

This comment has been minimized.

Copy link
@nosolosw

nosolosw Mar 30, 2020

Author Member

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.

lib/global-styles.php Outdated Show resolved Hide resolved

// Make object reference immutable
// so it stays the same in the function setters.
let styles = {};

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 12, 2020

Member

Should we move this inside the hook? And use the hook useRef with a start value of {}?

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Mar 30, 2020

Contributor

Also, the comment says "Make object reference immutable", but it's using let, not const.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

cc: @nosolosw should we do some update here?

packages/global-styles/src/provider.js Outdated Show resolved Hide resolved
packages/global-styles/src/renderer.js Outdated Show resolved Hide resolved
// so controls can modify the styles.
const { editEntityRecord } = useDispatch( 'core' );

const setColor = ( newStyles ) => {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 12, 2020

Member

We are generating new setters on every render. Should we consider the usage of useCallback/useMemo?

@nosolosw nosolosw force-pushed the try/global-styles-site-edit-ui-global branch 2 times, most recently from 7a9511d to d406c91 Mar 24, 2020
@johnstonphilip

This comment has been minimized.

Copy link
Contributor

johnstonphilip commented Mar 26, 2020

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.

Screen Shot 2020-03-26 at 4 42 36 PM

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Mar 27, 2020

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.

@nosolosw nosolosw force-pushed the try/global-styles-site-edit-ui-global branch 5 times, most recently from f601a58 to 5516a52 Mar 30, 2020
@nosolosw nosolosw force-pushed the try/global-styles-site-edit-ui-global branch from a1de624 to 413eadc Apr 1, 2020
</ComplementaryArea>
</>
) : (
<ComplementaryArea

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

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>
)

const { Slot: InspectorSlot, Fill: InspectorFill } = createSlotFill(
'EditSiteSidebarInspector'
);
function Sidebar() {
const hasGlobalStylesSupport = useSelect( ( select ) => {
return Object.keys( select( 'core/block-editor' ).getSettings() ).some(

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

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 = {};

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

cc: @nosolosw should we do some update here?

}

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

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

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.

This comment has been minimized.

Copy link
@nosolosw

nosolosw Apr 2, 2020

Author Member

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?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2020

Member

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.

This comment has been minimized.

Copy link
@nosolosw

nosolosw Apr 2, 2020

Author Member

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 #21346

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Apr 2, 2020

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.