Skip to content

Spacing presets: Add check for 0 spacing steps#43105

Merged
glendaviesnz merged 2 commits intotrunkfrom
fix/spacing-scale-zero-steps-notice
Aug 12, 2022
Merged

Spacing presets: Add check for 0 spacing steps#43105
glendaviesnz merged 2 commits intotrunkfrom
fix/spacing-scale-zero-steps-notice

Conversation

@glendaviesnz
Copy link
Copy Markdown
Contributor

@glendaviesnz glendaviesnz commented Aug 10, 2022

What?

Adds an early return from set_spacing_sizes if spacing.spaceScale.steps === 0.

Why?

Theme authors should be able to set this to 0 to prevent the generation of the core default spacing presets, and currently doing so causing an invalid valid error notice to be thrown.

How?

If pacing.spaceScale.steps === 0 return from method before spaceScale settings are validated

Testing Instructions

  • Add the following to your themes theme.json file settings section:
    "spacing": {
      "spacingScale": {
        "steps": 0
      }
    }
  • Load the editor or frontend and make double check that there are no --wp--preset--spacing--* CSS properties defined in the page source and that there is no PHP notice at the top of the page.

Screenshots or screencast

Before:
Screen Shot 2022-08-10 at 3 38 41 PM

After:
Screen Shot 2022-08-10 at 3 37 42 PM

andrewserong
andrewserong approved these changes Aug 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Good catch @glendaviesnz, this is working nicely for me!

Before, the following PHP Notice is output:

image

After, there is no notice and the presets aren't output. Setting to a value greater than 0 correctly still outputs the spacing sizes.

Not at all a blocker, but I was wondering if it's worth changing the check from 0 === $spacing_scale['steps'] to empty( $spacing_scale['steps'] ) so that any falsy value for steps would result in skipping the output without triggering an error? (E.g. 0, false, null, etc)

@andrewserong andrewserong added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 11, 2022
@glendaviesnz
Copy link
Copy Markdown
Contributor Author

glendaviesnz commented Aug 11, 2022

but I was wondering if it's worth changing the check from 0 === $spacing_scale['steps'] to empty( $spacing_scale['steps'] ) so that any falsy value for steps would result in skipping the output without triggering an error? (E.g. 0, false, null, etc)

The theory is that if a theme author doesn't want the core scale they explicitly set steps to 0, so it is a valid setting that will be documented .If they have set it to false or null it is invalid, so I was thinking we should throw the error to alert them to the fact that their settings are wrong - what do you think?

@andrewserong
Copy link
Copy Markdown
Contributor

andrewserong commented Aug 11, 2022

The theory is that if a theme author doesn't want the core scale they explicitly set steps to 0, so it is a valid setting that will be documented .If they have set it to false or null it is invalid, so I was thinking we should throw the error to alert them to the fact that their settings are wrong - what do you think?

That sounds reasonable to me, particularly because steps is expected to be a number. The only reason I thought some folks might wind up using false or null is because of potentially being used to using those values to switch other features off. From my perspective the warning is most useful when someone attempted to set correct settings but made an accidental mistake. But that's just the thoughts off the top of my head, don't let that stop you from merging!

andrewserong
andrewserong reviewed Aug 11, 2022
View reviewed changes
lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php Outdated
$spacing_scale = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'spacingScale' ), array() );

// If theme authors want to prevent the generation of the core spacing scale they can set their theme.json spacingScale.steps to 0.
if ( 0 === $spacing_scale['steps'] ) {
Copy link
Copy Markdown
Contributor

@andrewserong andrewserong Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, actually just another thought. This probably doesn't matter in practice because the base theme.json contains the 'steps' key, but if we keep the 0 check, should this line and the > 0 line below occur after the ! isset( $spacing_scale['steps'] ) check further down, to (theoretically) avoid an undefined index notice?

Copy link
Copy Markdown
Contributor Author

@glendaviesnz glendaviesnz Aug 12, 2022

Choose a reason for hiding this comment

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

yeh, makes more sense to check for invalid values first and then check for 0, have switched the order

@andrewserong andrewserong mentioned this pull request Aug 12, 2022
Merged
@andrewserong
Copy link
Copy Markdown
Contributor

andrewserong commented Aug 12, 2022 •
edited
Loading

Just retested and this is testing nicely for me, thanks for the update 👍

Again, not at all a blocker (do feel free to merge this PR as-is), but is there a reason we don't also allow opting out at the root feature level?

		"spacing": {
			"spacingScale": false
		},

Whichever way we recommend themes do it, it'd be good to update https://developer.wordpress.org/themes/advanced-topics/theme-json/#enabling-and-disabling-settings with the desired approach prior to 6.1. If / when we document it, we can add _**Note:** Since WordPress 6.1._ to flag the version the feature's targeting. (Since the feature is still in development, I don't think there's any rush to adding the docs, this can happily be a follow-up once more of the presets work has landed, but just thought I'd mention it)

@glendaviesnz glendaviesnz mentioned this pull request Aug 12, 2022
Open
18 tasks
@glendaviesnz
Copy link
Copy Markdown
Contributor Author

glendaviesnz commented Aug 12, 2022 •
edited
Loading

is there a reason we don't also allow opting out at the root feature level?

no explicit reason @andrewserong , just hasn't been considered to-date - have added a note to explore this to the tracking issue

@glendaviesnz glendaviesnz merged commit 89fe282 into trunk Aug 12, 2022
@glendaviesnz glendaviesnz deleted the fix/spacing-scale-zero-steps-notice branch August 12, 2022 03:27
@github-actions github-actions bot added this to the Gutenberg 14.0 milestone Aug 12, 2022
@andrewserong
Copy link
Copy Markdown
Contributor

andrewserong commented Aug 12, 2022

Nice one, thanks @glendaviesnz!

@ockham ockham mentioned this pull request Aug 26, 2022
Closed
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewserong andrewserong andrewserong approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

Assignees

No one assigned

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

Milestone

Gutenberg 14.0

Development

Successfully merging this pull request may close these issues.

2 participants

@glendaviesnz @andrewserong