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.

Sign up for GitHub

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

Jump to bottom

Fix how appearanceTools works #37254

Merged
merged 4 commits into from
Dec 10, 2021
Merged

Fix how appearanceTools works #37254

merged 4 commits into from
Dec 10, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Dec 9, 2021 •
edited
Loading

Fixes #37232
Follow-up to #36646

This PR fixes two things:

  • It looks like the mere presence of the appearanceTools flag was activating the others, while it should only be the case if appearanceTools was true.
  • There are some flags (such as blockGap) for which the null value is actually valid, so we need to detect whether they are set/unset using a different marker.

How to test

  • Activate TwentyTwentyTwo theme.
  • Add a post with a group and a few paragraphs within. Publish.

Verify that without appearanceTools things work as expected:

Verity that with appearanceTools active blockGap can be disabled:

@oandregal oandregal self-assigned this Dec 9, 2021
@oandregal oandregal added [Status] In Progress Tracking issues with work in progress Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended labels Dec 9, 2021
@oandregal oandregal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 9, 2021
@oandregal oandregal changed the title Only opt-in into tools if it is actually true Fix blockGap cannot be disabled Dec 9, 2021
@oandregal oandregal mentioned this pull request Dec 9, 2021
Closed
@oandregal oandregal changed the title Fix blockGap cannot be disabled appearanceTools: only opt-in if it is true Dec 9, 2021
@oandregal oandregal marked this pull request as ready for review December 9, 2021 16:18
@oandregal oandregal requested a review from ndiego December 9, 2021 16:19
@oandregal
Copy link
Member Author

oandregal commented Dec 9, 2021 •
edited
Loading

@ndiego This won't fix the issue you reported at #37232 because it's different. I'll comment there. This fix is still necessary, though.

Let me rephrase: this PR fixes a couple of issues with appearanceTools that affected the blockGap. What I mean is that blockGap: false doesn't work as expected and needs to be investigated separately.

@oandregal oandregal changed the title appearanceTools: only opt-in if it is true Fix how appearanceTools works Dec 9, 2021
@ndiego
Copy link
Member

ndiego commented Dec 9, 2021

Thanks for this @oandregal, I have tested and confirm that this fix works as intended!

andrewserong
andrewserong approved these changes Dec 10, 2021
View reviewed changes
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 the ping and fix @oandregal!

✅ Tested that the appearanceTools setting only affects the opt-ins when set to true.
✅ Tested that null is a valid override value in settings.spacing.blockGap and that the settings overrides appear to work correctly with the appearanceTools setting.

LGTM! 🚀

aaronrobertshaw
aaronrobertshaw approved these changes Dec 10, 2021
View reviewed changes
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.

Nice work as always @oandregal 👍

✅ code changes LGTM
✅ unit tests pass
✅ appearanceTools only opts-in other features when set to true
✅ null override for settings.spacing.blockGap works

@Mamaduka Mamaduka merged commit 47f6ee8 into trunk Dec 10, 2021
@Mamaduka Mamaduka deleted the fix/block-gap-disabled branch December 10, 2021 08:31
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 10, 2021
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 13, 2021
noisysocks pushed a commit that referenced this pull request Dec 13, 2021
@oandregal @noisysocks
Fix how appearanceTools works (#37254)
fdfffa4 
* Only opt-in into tools if it is actually true

* Fix lint issues

* Allow null values in the props

* Fix lint issue
@noisysocks noisysocks mentioned this pull request Dec 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronrobertshaw aaronrobertshaw aaronrobertshaw approved these changes

@andrewserong andrewserong andrewserong approved these changes

@ndiego ndiego ndiego approved these changes

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

Assignees

@oandregal oandregal

Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 12.2
Development

Successfully merging this pull request may close these issues.

In 12.1, blockGap appears partially enabled when it shouldn't be
6 participants
@oandregal @ndiego @andrewserong @aaronrobertshaw @Mamaduka @noisysocks