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

Fix: Add ability to opt out of Core color palette #36447

Closed

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented Nov 12, 2021

Description

Add the ability for themes using theme.json to opt out of showing the "Core" color Palette and Core Gradients.

{
	"version": 1,
	"settings": {
		"color": {
			"corePalette": false,
			"coreGradients": false
		}
	}
}

Closing #36407

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@fabiankaegy fabiankaegy linked an issue Nov 12, 2021 that may be closed by this pull request
@fabiankaegy
Copy link
Member Author

fabiankaegy commented Nov 12, 2021

@oandregal Hey 👋

Would love to get some feedback on this initial approach. In my testing, it does what we talked about in #36407 but I am not sure where the best place to execute this is.

Would love to get some feedback on it :)

Thanks in advance!

@oandregal
Copy link
Member

Nice! Thanks for the quick turnaround.

Can we do it in a way that the changes are scoped to WP_Theme_JSON but not the resolver?

@fabiankaegy
Copy link
Member Author

@oandregal I looked into it for a while and currently don't know how exactly we can get there. The issue I am running into is that there is no hook or anything that executes after the different theme.json files are merged.

I'll continue to look into it though :)

If anyone has any suggestions or ideas I'm all open for input :)

@fabiankaegy fabiankaegy self-assigned this Nov 12, 2021
@fabiankaegy fabiankaegy added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Bug An existing feature does not function as intended labels Nov 12, 2021
@oandregal
Copy link
Member

Would you be able to add some tests for this new behavior? It'd help us to identify/clarify that this works.

My thinking is as follows: we can inline the current maybe_remove_core_color_palette_and_gradients logic into the WP_Theme_JSON.merge method: if the setting exists, we empty the core palette. We need to merge theme's settings on top of core's settings, hence, the core palette is going to be present when the incoming data is the theme's.

@fabiankaegy fabiankaegy marked this pull request as ready for review November 14, 2021 12:49
@fabiankaegy
Copy link
Member Author

@oandregal I moved the function call into the merge function so it is now contained within the WP_Theme_JSON_Gutenberg class.

I also added some test cases for it :)

@fabiankaegy
Copy link
Member Author

One edge case that is currently not being accounted for is you opting out on the global level but then opting-in on the block level.

{
    "version": 1,
    "settings": {
      "color": {
        "corePalette": false,
        "coreGradiens": false
      },
      "blocks": {
        "core/heading": {
          "color": {
            "corePalette": true,
            "coreGradients": true
          }
        }
      }
    }
  }

Since the original theme.json from Gutenberg doesn't contain an explicit core palette for each block they by default would use the global one. But that doesn't exist anymore when you have opted out.

@oandregal
Copy link
Member

@fabiankaegy that's a good point, thanks for bringing it up. We want to retain the cascade behavior. The other thing this approach makes harder to do is that we still want the classes and CSS custom properties for the core palette to be enqueued everywhere: even if the user does not use them, patterns still can.

So, I was thinking we need to approach it from a different direction. It sounds like we can still have the corePalette flag, but use it in the client instead. The PanelColorGradientSettingsMultipleSelect component (see) would use the flag to pass an empty array for the core color flags.

Would you think you'd have the time to explore that approach instead?

@fabiankaegy
Copy link
Member Author

@oandregal Yeah I think that approach makes more sense. I have created a follow up PR here: #36492

@oandregal
Copy link
Member

Closing in favor of #36492

@oandregal oandregal closed this Nov 15, 2021
@oandregal oandregal deleted the fix/add-ability-to-dissable-core-color-palette branch November 15, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing ability to remove/override "Core" color palette
2 participants