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 Color Palette editor default to theme/core pallete. #26783

Conversation

jorgefilipecosta
Copy link
Member

Currently, we are not defaulting the global styles color palette editor to the theme colors, so if the user just wanted to make a small change to color it is hard. This PR makes us default to the theme colors.

How has this been tested?

I enabled the 2021 blocks theme.
I verified that the color palette editor appears with the theme colors by default.
I verified that If I changed the second color also used for text color, the text color of the theme changed.
I verified that If I changed the fourth color also used as the background color, the background color of the theme changed.

(Names of the colors are not appearing because of a theme issue I'm going to fix in the theme itself)

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 6, 2020
@github-actions
Copy link

github-actions bot commented Nov 6, 2020

Size Change: +22 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/components/index.js 171 kB +21 B (0%)
build/edit-site/index.js 22.8 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.91 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.89 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.52 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.43 kB 0 B
build/edit-post/style.css 6.42 kB 0 B
build/edit-site/style-rtl.css 3.95 kB 0 B
build/edit-site/style.css 3.95 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 713 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.77 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.83 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.83 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@oandregal
Copy link
Member

👋 This is a lovely improvement! I'm eager to get this in. Here are the things that I think we should fix first:

  • Theme colors slugs should remain stable. When the user changes a color name provided by the theme, it also changes its slug. This breaks things in unpredictable ways. Example: change the Green color name to Green base and see how the background loses its color.

  • Theme colors should be always present in the stylesheet, even if we allow the user to remove them from the visual palette. Example: remove the Green color from the palette and see how the background loses its color.

Essentially, the theme colors should always generate the CSS Custom Properties and classes that the theme expects, even if we allow the user to change the color value, the color name, or removed them from the UI.

Other thoughts that I have are (non-blocking, can be addressed in subsequent PRs):

  • The color palette editor needs a "reset" button on its own that gets back the original theme colors.

  • When the user changes a color (name or value) all the color palette is stored in the user CPT. I wonder if a better approach would be to store only the colors the user tweaked. I can only foresee one case when the current approach can be problematic: if the theme updates its color palette in a version update, any user that tweaked them won't have the updated palette. Doesn't seem something to prioritize now, but wanted to share anyway.

@jorgefilipecosta jorgefilipecosta force-pushed the update/global-styles-color-pallete-editor-default-to-theme-pallets branch from a4e5cef to 6526a52 Compare November 12, 2020 12:12
@jorgefilipecosta
Copy link
Member Author

Hi @nosolow,
Thank you for the review 👍

Theme colors slugs should remain stable. When the user changes a color name provided by the theme, it also changes its slug. This breaks things in unpredictable ways. Example: change the Green color name to Green base and see how the background loses its color.

I updated the code to make sure when the user changes the theme color's name; the slug is not automatically changed.

Theme colors should be always present in the stylesheet, even if we allow the user to remove them from the visual palette. Example: remove the Green color from the palette and see how the background loses its color.

I agree with this. In fact, I think even core color classes and variables should also be present all the time.
But this is a considerable architectural change. Right now, we merge a tree with core, theme, and user styles and settings, and then we generate preset classes and variables on the result. We need to change things on the server to merge the presets in a different way. This change is not related to this PR. The way things are merged is not being changed here. And we have #26803 changing things at the engine level. To avoid needing to rebase things, I guess we can address the merging of presets in a different PR after #2680 is merged?

@jorgefilipecosta jorgefilipecosta merged commit f7c91c8 into master Nov 13, 2020
@jorgefilipecosta jorgefilipecosta deleted the update/global-styles-color-pallete-editor-default-to-theme-pallets branch November 13, 2020 16:43
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 13, 2020
@jorgefilipecosta
Copy link
Member Author

I merged this PR so the follow-ups can continue and we avoid rebase and conflicts. If there is any additional feedback I'm happy to iterate on follow up PR's.

@oandregal oandregal mentioned this pull request Nov 26, 2020
82 tasks
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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants