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

Theme Export: Restore appearanceTools when exporting a theme #39840

Merged
merged 14 commits into from
Apr 7, 2022

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Mar 29, 2022

What?

When you export a theme that declares support for appearanceTools in theme.json we should keep this declaration rather than converting it to the properties that this maps to. Fixes #39464.

Why?

This will make it easier for theme developers to use the theme export tool, and also enable them to easy opt in to more settings in the future.

How?

By overloading the do_opt_in_into_settings method in WP_Theme_JSON_Gutenberg and creating a new one called do_opt_out_of_settings which does the reverse.

Testing Instructions

  • Switch to a theme that uses the appearanceTools property in theme.json (e.g. TT2)
  • Go to the Site Editor and export the theme
  • Confirm that the appearanceTools remain in the theme.json file under settings
  • Confirm that the other properties aren't declared:
settings.border.color
settings.border.radius
settings.border.style
settings.border.width
settings.color.link
settings.spacing.padding
settings.spacing.blockGap
settings.spacing.margin

Notes

There are also some other settings that are opted in to (e.g. settings.color.custom, settings.color.customGradient) but this has a different cause so we should look at it in a different PR...

cc @WordPress/block-themers

if ( array_key_exists( 'appearanceTools', $theme_json['settings'] ) ) { // Is it safe to assume that 'settings' always exsits?
foreach ( static::TO_OPT_IN as $path ) {
// Remove the path.
if ( ! empty( $theme_json['settings'][ $path[ 0 ] ][ $path[ 1 ] ] ) ) {
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 really hard to read because of the $path[ 0 ] thing, but I'm not sure what would be a better approach...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like

foreach ( static::TO_OPT_IN as list( $group, $prop ) ) {
	if ( ! empty( $theme_json['settings'][ $group ][ $prop ] ) ) {

@draganescu
Copy link
Contributor

Why overload do_opt_in_into_settings?

@scruffian
Copy link
Contributor Author

Why overload do_opt_in_into_settings?

So that it can use the new class variable - the old one just has that array hardcoded into the function.

@scruffian scruffian force-pushed the update/theme-export-restore-appearance-settings branch from 1d5d27c to ced68ae Compare April 6, 2022 09:07
protected static function do_opt_in_into_settings( &$context ) {
foreach ( static::TO_OPT_IN as $path ) {
// Use "unset prop" as a marker instead of "null" because
// "null" can be a valid value for some props (e.g. blockGap).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can unset prop be, or ever become, a valid value? I know it's unlikely for, say, color – but still. How about an object? It should also have a unique identity.

Copy link
Member

Choose a reason for hiding this comment

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

All existing opt-ins are booleans (or null).

@oandregal oandregal force-pushed the update/theme-export-restore-appearance-settings branch from 9a5b652 to 790bbbc Compare April 6, 2022 18:31
@oandregal
Copy link
Member

I was looking into this and thought I'd push a couple of changes to:

  • add unit tests
  • implement the behavior for both top-level and block-level settings

@scruffian
Copy link
Contributor Author

Thanks @oandregal. I fixed a few linting issues...

@scruffian scruffian force-pushed the update/theme-export-restore-appearance-settings branch from d289eba to 655b216 Compare April 7, 2022 10:37
@scruffian scruffian force-pushed the update/theme-export-restore-appearance-settings branch from 655b216 to ffb8f1a Compare April 7, 2022 11:05
@scruffian scruffian merged commit a618eaf into trunk Apr 7, 2022
@scruffian scruffian deleted the update/theme-export-restore-appearance-settings branch April 7, 2022 11:59
@github-actions github-actions bot added this to the Gutenberg 13.0 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

Exported JSON loses 'appearanceTools' abstraction
4 participants