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

Spacing Support: Hide UI for disabled properties #31726

Merged

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented May 11, 2021

Description

Fixes issue #31725
The Spacing block support has support for both padding and margin independently; it is possible for a theme to opt in to only one or the other. Currently the UI for both is displayed as long as the block supports that feature, and the theme supports either feature. This PR updates the block support to only display the UI if it is supported by both the block and the theme.

How has this been tested?

Manually

To test, you need a block that supports both padding and margin. I used Site Title and updated the block.json to include:

"supports": {
    // ...other supports
    "spacing": {
        "padding": true,
        "margin:": true
    }
}

You also need to update your theme.json to enable customPadding, but disable customMargin:

"settings": {
   "spacing": {
        "customPadding": true,
        "customMargin": false
    }
}
  1. Open the site editor and select the Site Title block. View the Inspector Controls and verify that only the Padding controls display.
  2. Update the theme.json to set both customMargin and customPadding to true. Verify that now both Margin & Padding controls display.
  3. Update theme.json to only enable customMargin, and verify that now only Margin controls display.
  4. -Update theme.json to disable both padding and margin, and verify that the Spacing panel is not rendered at all.

Screenshots

The Spacing panel when both Padding & Margin are supported by both the block and the theme:
Screen Shot 2021-05-11 at 1 42 56 PM

The Spacing panel when the block supports both Padding & Margin, but the theme only supports Margin:
Screen Shot 2021-05-11 at 12 53 50 PM

Types of changes

Bug fix (non-breaking change which fixes an issue)

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).

@stacimc stacimc marked this pull request as ready for review May 11, 2021 20:54
@stacimc stacimc requested a review from ellatrix as a code owner May 11, 2021 20:54
@aaronrobertshaw aaronrobertshaw self-requested a review May 12, 2021 01:01
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @stacimc!

The spacing support controls need to check both whether the block opted into the support feature as well as if the theme disabled the UI for that feature.

I mistakenly thought the disabled check didn't take into account the block's support flags and pushed a commit adding it as well. My mistake. That's been reverted. Sorry.

This now tests well for me in both the block editor and site editor.

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Also working well for me! 🙇

@aaronrobertshaw aaronrobertshaw merged commit 4670e4a into WordPress:trunk May 12, 2021
@github-actions github-actions bot added this to the Gutenberg 10.7 milestone May 12, 2021
@oandregal oandregal mentioned this pull request May 12, 2021
82 tasks
@stacimc stacimc deleted the fix/spacing-support-ui-when-disabled branch May 12, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants