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/Dimensions Supports: Separate spacing from dimensions for compatibility purposes #34059

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

aaronrobertshaw
Copy link
Contributor

Related: #32392

Description

This PR addresses an issue of compatibility with core created by the renaming of spacing to dimensions support in #32392.

  • The previous version of spacing.php has been restored
  • The dimensions.php server-side hook remains in place ready to accept height and width support
  • No changes to the block.json or theme.json support keys have been made
    • spacing contains padding and margin support
      • block.json and theme.json both use the stabilised spacing key.
    • dimensions will contain height, width, and possibly min/max versions of those.
      • block.json uses an __experimentalDimensions key
      • theme.json uses a dimensions key

How has this been tested?

  • npm run test-unit:watch packages/block-editor/src/hooks/test/style.js
  • npm run wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose --filter stylesheet /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.php'
  • Manually by:
    • opting into both padding and margin support under spacing for the group block
    • opting into both padding and margin support for the dynamic site tagline block
    • confirming dimensions hook is in place but takes no effect for the moment until the height and width supports add their application of styles within it

Types of changes

Bug fix.

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

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. labels Aug 13, 2021
@aaronrobertshaw aaronrobertshaw self-assigned this Aug 13, 2021
Copy link
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.

Thanks for following up on this @aaronrobertshaw!

✅ Confirmed that the diff between the old spacing.php and the re-introduced spacing.php is just the doc comment added at the top of the file
✅ Minimalist dimensions.php file looks good and ready for us to add in height and width supports 👍
✅ Smoke tested that the existing spacing supports still work as expected (padding and margin in Group block, and the Button block's padding which skips serialization)

Also, IMO it feels like it'll be useful having the width and height controls separately in the dimensions support as that way it'll be possible to have slightly more granular control over skipping serialization between height/width and padding/margin 👍

@aaronrobertshaw
Copy link
Contributor Author

Also, IMO it feels like it'll be useful having the width and height controls separately in the dimensions support as that way it'll be possible to have slightly more granular control over skipping serialization between height/width and padding/margin

That's a good point. Which I think might tip the decision in favour of this approach rather than unifying the support keys.

This ignore will be removed once height block support is added as the parameter will be used then.
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.

Bit late, but also tested.

✔️ Controls appear as expected.
✔️ Changes reflected in the frontend
✔️ Tests pass

Thank you!

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Haven't tested but the code changes look good.

@oandregal oandregal merged commit 0428dcc into trunk Aug 13, 2021
@oandregal oandregal deleted the update/spacing-and-dimensions-server-side-hooks branch August 13, 2021 07:27
@github-actions github-actions bot added this to the Gutenberg 11.4 milestone Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants