theme.json: adds a setting property that enables some other ones

[oandregal]

Fixes #36187

As per the conversation in the issue linked, this PR adds a setting property whose function is to opt-in into a few others.

The result is that a theme.json like this one:

{
  'version': 1,
  'settings': {
    'appearance': true
  }
}

is the same as this other:

{
  'version': 1,
  'settings': {
    'border': {
      'color': true,
      'radius': true,
      'style': true,
      'width': true
    },
    'spacing': {
      'margin': true,
      'padding': true,
      'units': true
    },
    'typography': {
      'customFontSize': true,
      'lineHeight': true
    } 
  }
}

TODO

The name of the setting

At the moment, the setting's name is appearance as seen in the issue linked. I'd suggest changing it to a more obvious name, for example, allowOptIn. It'd also free the appearance key for our use in the future (it looks like a legit key that could be used).

Confirm which settings need to be enabled

At the moment, they are:

• border: color, radius, style, width
• spacing: margin, padding, units
• typography: customFontSize, lineHeight

What should happen with this theme.json?

{
  'version': 1,
  'settings': {
    'appearance': true,
    'typography': {
      'lineHeight': false
    }
  }
}

Option 1, the new property enables everything and disregards the existing values for the properties it enables:

{
  'version': 1,
  'settings': {
    'border': {
      'color': true,
      'radius': true,
      'style': true,
      'width': true
    },
    'spacing': {
      'margin': true,
      'padding': true,
      'units': true
    },
    'typography': {
      'customFontSize': true,
      'lineHeight': true
    } 
  }
}

Option 2, the new property respects the false values of the properties it enables:

{
  'version': 1,
  'settings': {
    'border': {
      'color': true,
      'radius': true,
      'style': true,
      'width': true
    },
    'spacing': {
      'margin': true,
      'padding': true,
      'units': true
    },
    'typography': {
      'customFontSize': true,
      'lineHeight': false
    } 
  }
}

At the moment it works like option 1, which goes in line with the spirit of the original issue, as I see it: to let some themes to opt-in into any new settings Gutenberg adds.

I also see how 2 can be convenient.

Thoughts?

[carolinan]

Option 2 as it saves us a lot of typing and space in theme.json.

[MaggieCabrera]

Option 2 seems the most logical to me, and what @carolinan said too

[fabiankaegy]

I would vote for Option 2. I'm thinking of it kind of in the same way we think of the global vs the block scope for settings.

If the setting does not exist in the block level or on the global level it should use the default set by "appearance": true

[kjellr]

Yeah, option 2 makes sense to me as well. That way a theme author could easily opt into everything except one or two features if that's what they wanted to do.

[kjellr]

At the moment, the setting's name is appearance as seen in the issue linked. I'd suggest changing it to a more obvious name, for example, allowOptIn. It'd also free the appearance key for our use in the future (it looks like a legit key that could be used).

A few suggestions:

• "allAppearance": true
• "allAppearanceSettings": true
• "appearanceOptions": true
• "optInSettings": true
• "optInAll": true

I think that second one seems most understandable to me.

[jffng]

+1 option 2.

"allAppearanceSettings": true

I prefer a more explicit naming like this ^ or "enableAllAppearanceControls".

[jffng]

Small typo, the key name is probably going to change the name anyways:

Suggested change

	There's one special setting property, `appareance`, which can be a boolean and its default value is true. When this is enabled, the following setting properties will be on by default:
	There's one special setting property, `appearance`, which can be a boolean and its default value is true. When this is enabled, the following setting properties will be on by default:

[colorful-tones]

At the moment, the setting's name is appearance as seen in the issue linked. I'd suggest changing it to a more obvious name, for example, allowOptIn.

I'm on the fence between "allowOptin": true and "optInAll": true. If we're to go with Option 2 then it might be ideal to go with: "enableOptinAll": true, which kind of helps try to give some heuristics to theme authors that they can still do 'lineHeight': false.

It'd also free the appearance key for our use in the future (it looks like a legit key that could be used).

💯 agree that appearance has more potential down the road for a better key for a better feature.

Option 2 seems ideal to me. Opt in to all, and then opt out of any few stragglers that theme author might want to.

At the moment, they are:

• border: color, radius, style, width
• spacing: margin, padding, units
• typography: customFontSize, lineHeight

I think the supported properties are great coverage:

[aaronrobertshaw]

+1 for Option 2.

At the moment, the setting's name is appearance as seen in the issue linked. I'd suggest changing it to a more obvious name, for example, allowOptIn.

I'm not sure allowOptIn fits given theme authors can already opt-in, just individually for each feature.

If we were going with Option 1, something like allAppearanceSettings or allDesignFeatures etc. sound more fitting to me. Capturing the intent of the flag if going with Option 2 is trickier. Perhaps something involving optInByDefaultcould communicate its purpose?

At the moment, they are:

border: color, radius, style, width
spacing: margin, padding, units
typography: customFontSize, lineHeight

My understanding is that use of margin support is going to be rather selective moving forward and we'll probably be leaning more on using blockGap support. If that's the case, would it also pay to include blockGap in the spacing supports enabled via this new flag?

All in all, this looks like it will be a great enhancement 👍

[noisysocks]

I see this is approved so swooping in to merge it in time for WP 5.9 beta 1 😀

[noisysocks]

Sorry @oandregal, merging this caused legitimate PHP unit test failures on trunk. I'm not sure why they were passing in the PR. Please revert this revert, fix up the tests, and I'll backport that into the 5.9 branch in time for Beta 2.

[oandregal]

Second try at #36646

[oandregal]

#36646 has been merged.