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

Make the core color palette opt-in for themes with not theme.json #36496

Merged
merged 4 commits into from Nov 18, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 15, 2021

Follow-up to #36492 #35970

In #35970 we introduced the ability to show three different palettes (default, theme, user) in the color UI component. We also have a couple of flags for themes to disable this should they want (corePalette, coreGradients). However, by introducing this change, themes that don't use theme.json will now see the default colors as well.

This PR introduces the following changes:

  • Hides the default palette for themes with no theme.json that declare its own palette.
  • Adds new theme supports for themes with no theme.json to opt-in into the default solid and gradient palettes, so they can show both the default and the theme one:
    • default-color-palette which mimic the existing editor-color-palette theme supports
    • default-gradient-presets which mimics the existing editor-gradient-presets theme support

How to test

Test a theme that does not provide a palette:

  • Use Storefront.
  • Load the editor and verify that the color component shows the default palette.

Test a theme that provides a palette:

  • Use TwentyTwentyOne.
  • Load the editor and verify that the color component only shows the theme solids&gradients but not the default ones (they'll be shown without this PR).

Test a theme that provides a palette and opts-in into the core palette:

  • Use TwentyTwentyOne.
  • Add add_theme_support( 'default-color-palette' ) and add_theme_support( 'default-gradient-presets' ) in its functions.php.
  • Load the editor and verify that the color component shows both the default and the theme palettes for solids & gradients.

@oandregal oandregal self-assigned this Nov 15, 2021
@oandregal oandregal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 15, 2021
@oandregal oandregal added this to 👀 Needs review in WordPress 5.9 Must-Haves via automation Nov 15, 2021
@fabiankaegy
Copy link
Member

Closes #36489

@fabiankaegy fabiankaegy linked an issue Nov 15, 2021 that may be closed by this pull request
@oandregal oandregal changed the title Disable the core color palette when if the theme does not have a theme.json Disable the core color palette when the theme does not have a theme.json Nov 15, 2021
Comment on lines +121 to +122
// The "default" theme doesn't have theme.json support.
switch_theme( 'default' );
Copy link
Member

Choose a reason for hiding this comment

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

It might not be accurate in the future, but it works for now.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

It works as expected.

Before After
CleanShot 2021-11-16 at 14 39 40 CleanShot 2021-11-16 at 14 40 15

@oandregal oandregal force-pushed the fix/disable-core-palette-when-no-theme-json branch from b4c25a0 to d3906db Compare November 18, 2021 12:11
@oandregal
Copy link
Member Author

This is not ready to land. We need to also cover the case in which the theme does not provide any theme palette: in that case, we still need to show the core one. Updated PR description with instructions for testing.

@Mamaduka
Copy link
Member

@oandregal, I'm going to add the "In Progress", so we don't merge it by accident 😄

@Mamaduka Mamaduka added the [Status] In Progress Tracking issues with work in progress label Nov 18, 2021
@oandregal oandregal removed the [Status] In Progress Tracking issues with work in progress label Nov 18, 2021
@oandregal
Copy link
Member Author

@fabiankaegy @Mamaduka This is now ready to test.

I've run this by some folks (Matías and others) and they think it's best to add new theme supports that classic themes can use to opt-in into this, so I've done that as well. Another point of feedback was that we should rename core into defaults everywhere. I'll create a separate follow-up PR for that change.

@oandregal
Copy link
Member Author

I'm going to go ahead and merge this one to unblock #36622 Please, feel free to do post-merge reviews and I'll address any issue left.

@oandregal oandregal merged commit 475753b into trunk Nov 18, 2021
WordPress 5.9 Must-Haves automation moved this from 👀 Needs review to ✅ Done Nov 18, 2021
@oandregal oandregal deleted the fix/disable-core-palette-when-no-theme-json branch November 18, 2021 16:35
@oandregal oandregal changed the title Disable the core color palette when the theme does not have a theme.json Make the core color palette opt-in for themes with not theme.json Nov 18, 2021
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 18, 2021
@oandregal oandregal added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 18, 2021
@Mamaduka
Copy link
Member

I did another test run and works as expected 👍

@noisysocks noisysocks removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 22, 2021
noisysocks pushed a commit that referenced this pull request Nov 22, 2021
…json` (#36496)

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@steveangstrom
Copy link

This is not fixed in Gutenberg 12.0.1 . Still broken there. I have a wall of ugly "core".

I assume that's intentional?

Is there a timeline for this patch to make it into the released plugin?

@oandregal
Copy link
Member Author

Hey Steve, this is going to be part of the 12.1 release, which should be a few days away.

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] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Core palette can no longer be overridden from PHP
5 participants