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

Use inline styles instead of CSS variables for the style attribute #21428

Merged
merged 7 commits into from
Apr 8, 2020

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Apr 6, 2020

CSS attribues are nice for properties that are used in several blocks but they create a cascade issue for things like backgrounds, colors. When I set a background color on a group, I don't want its inner buttons to inherit it.

@youknowriad youknowriad added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Apr 6, 2020
@youknowriad youknowriad requested review from mtias and oandregal April 6, 2020 15:30
@youknowriad youknowriad self-assigned this Apr 6, 2020
lib/client-assets.php Outdated Show resolved Hide resolved
Comment on lines 69 to 100
const mappings = {
lineHeight: [ 'typography', 'lineHeight' ],
backgroundColor: [ 'color', 'background' ],
color: [ 'color', 'text' ],
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the important part (mapping style objects to inline styles) each time we add a new config/CSS variable, we should the right mapping here.

@github-actions
Copy link

github-actions bot commented Apr 6, 2020

Size Change: -258 B (0%)

Total Size: 889 kB

Filename Size Change
build/block-editor/index.js 102 kB -66 B (0%)
build/block-library/index.js 110 kB -4 B (0%)
build/block-library/style-rtl.css 7.42 kB -115 B (1%)
build/block-library/style.css 7.43 kB -118 B (1%)
build/components/index.js 195 kB -15 B (0%)
build/editor/editor-styles-rtl.css 428 B +29 B (6%) 🔍
build/editor/editor-styles.css 431 B +31 B (7%) 🔍
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.79 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 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 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 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.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.05 kB 0 B
build/edit-navigation/index.js 2.75 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.17 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 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/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.6 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad force-pushed the update/opt-in-global-styles branch from 5c09c8c to 6d21c0b Compare April 7, 2020 12:28
@youknowriad youknowriad requested review from nerrad and ntwb as code owners April 7, 2020 12:41
@youknowriad youknowriad changed the title Add the Global Styles support to the blocks in an opt-in way Use inline styles instead of CSS variables for the style attribute Apr 7, 2020
@pinarol pinarol requested a review from dratwas April 7, 2020 15:08
@oandregal
Copy link
Member

We need to replicate the patterns introduced at #21131 and #21132 with this PR and update their JSON as well.

@youknowriad
Copy link
Contributor Author

@nosolosw Good catch, it should be fixed.

@oandregal
Copy link
Member

The following bit in packages/block-library/src/button/style.scss could be removed as well:

.wp-gs .wp-block-button__link:not(.has-background) {
	background-color: var(--wp--color--primary);
}

I meant to delete it in some PRs that didn't land yet because it uses the wp-gs class that we don't want to expose atm. I guess it's fine to do it here as well to remove any trace of CSS vars just yet (although not really necessary if you don't want, as it is previous to the addition of the colors hook).

@oandregal
Copy link
Member

We also need to bring back these editor-styles.

@youknowriad
Copy link
Contributor Author

@nosolosw I'm still curious to understand why. Maybe some specificity issue somewhere.

@oandregal
Copy link
Member

I'll check. I believe we have something in the dev version of the plugin that resets the color palette in order to avoid failures in e2e tests due to theme changes. I wonder if it's related. Did you check if you had the same color palette before and after using the plugin?

I believe I ran into what you mention myself. However, I think it was due to the environment and not the plugin, though? I mention it because I'm not using any env bundled with Gutenberg for this testing.

Also: yes, same palette. In case it rings any bell:

200407-190200-palette-store-wp-5-4

@oandregal
Copy link
Member

oandregal commented Apr 7, 2020

OK, I think I got it. One thing that this PR modifies from WP 5.4 is that it doesn't inline the styles in the editor for presets (only for custom styles). It looks like that isn't enough for 2019 and this declaration kicks-in and takes precedence over the classes added by the presets.

Perhaps we can add that behavior back, even for the presets?

@youknowriad
Copy link
Contributor Author

I'm pretty sure we didn't change anything about how presets are applied though. with or without this PR.

@oandregal
Copy link
Member

I've come to this today with fresh eyes, and this is what I've observed: what I reported before happens for old and new content using the TwentyNineteen theme --- in this branch, but also in master. I've pinpointed this issue to the addition of color hooks to the paragraph block (before that it works nicely).

This PR is a great addition that is good to land as soon as we can to avoid potential conflicts (like what happened for patterns). So, I wonder how do you want to proceed: shall we merge this one and investigate the issue separately? I'm happy to approve if so.

@youknowriad
Copy link
Contributor Author

So, I wonder how do you want to proceed: shall we merge this one and investigate the issue separately?

Yes, let's do that, I'm planning to investigate the other issue today too.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

I've adapted #21431 to work with this.

@youknowriad youknowriad merged commit c19d2d9 into master Apr 8, 2020
@youknowriad youknowriad deleted the update/opt-in-global-styles branch April 8, 2020 10:00
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 8, 2020
@youknowriad
Copy link
Contributor Author

@nosolosw @kjellr

I think I know what's happening. Previously we used to add the colors chosen as inline styles in the editor markup even if you choose a preset, while with the new approach, we're getting closer to the frontend and if you choose a palette color, we just apply the className. Twenty nineteen is not loading the entire palette color stylesheet in the editor which means when it's just the class that is added, the colors are not applied properly.

I believe the new approach is the right one since it matches frontend and backend more closely but it means 2019 needs to be patched to load the palette colors on the editor too.

@kjellr
Copy link
Contributor

kjellr commented Apr 8, 2020

I believe the new approach is the right one since it matches frontend and backend more closely but it means 2019 needs to be patched to load the palette colors on the editor too.

Ok cool, that's doable. Are other themes having this problem too?

@youknowriad
Copy link
Contributor Author

Are other themes having this problem too?

Can't tell for sure, I can say that it works well in 2020

@kjellr
Copy link
Contributor

kjellr commented May 7, 2020

Previously we used to add the colors chosen as inline styles in the editor markup even if you choose a preset, while with the new approach, we're getting closer to the frontend and if you choose a palette color, we just apply the className. Twenty nineteen is not loading the entire palette color stylesheet in the editor which means when it's just the class that is added, the colors are not applied properly.

I believe the new approach is the right one since it matches frontend and backend more closely but it means 2019 needs to be patched to load the palette colors on the editor too.

Just noting that I opened a core issue for this...

https://core.trac.wordpress.org/ticket/50120

... and also discovered that it requires patches for almost all of the core themes. This makes me a little nervous that this change might end up resulting in widespread breakage of the color palettes in the editor. The fact that 2020 includes its custom colors in the editor styles might just be an anomaly.

@youknowriad
Copy link
Contributor Author

@kjellr yeah, we should probably just add these inline styles again but I'm wondering how we can nudge folks to actually enqueue these styles in the editor too.

@kjellr
Copy link
Contributor

kjellr commented May 8, 2020

I'm wondering how we can nudge folks to actually enqueue these styles in the editor too.

For a lot of these sorts of updates, I see block-based themes & global styles as a potential changeover point. If your theme has block templates and a theme.json (or whatever we call it), then perhaps that's also an indication that your theme is ready to opt into future-forward updates like this and the alternate buttons markup.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants