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

Button: Add theme.json support for toggling Width settings panel #42079

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

roseg43
Copy link
Contributor

@roseg43 roseg43 commented Jun 30, 2022

What?

This allows theme developers to manage support for the Width settings panel on the Button block using theme.json.

Why?

Unlike most spacing-related settings, this panel and associated block option requires that theme developers explicitly add support for the classes it applies to Button blocks. With no way to define support at the theme level for this setting, developers are required to either leave in a panel with options that don't do anything, or add support for it. The latter option is often not feasible given corporate branding requirements and/or rigidly defined design libraries.

All other spacing and sizing related options are toggle-able via theme.json. As such, Button width settings should be consistent with its related options.

Fixes #38767. See #19796

How?

We toggle visibility for the panel in Button's edit component using useSetting() to grab the new setting added to theme.json. Appropriate entries have been added for the new setting to both the theme.json schema as well as the core-blocks and theme-json-living documentation.

Support for this feature can now be toggled via the spacing.width property in theme.json

Testing Instructions

  1. Open a Post or Page
  2. Add a Button block
  3. Confirm that the Width settings panel is visible
  4. In theme.json for the currently active theme, add spacing.width: false to the settings.spacing property
  5. Reload the Post/Page editor, and confirm that the Width settings panel on the Button block is no longer visible.
  6. Confirm that the same is true if spacing.width: false is set at the block setting level in theme.json

@skorasaurus skorasaurus added the [Block] Buttons Affects the Buttons Block label Jun 30, 2022
@roseg43 roseg43 marked this pull request as ready for review July 1, 2022 13:38
@@ -219,6 +219,11 @@
"type": "boolean",
"default": false
},
"width": {
Copy link
Member

@skorasaurus skorasaurus Jul 1, 2022

Choose a reason for hiding this comment

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

Thank you; the button width is no longer available by default, remaining consistent with the other spacing settings; so it did not appear; even when I did not set any value to spacing.width in my theme.json (Step 4)

Copy link
Member

Choose a reason for hiding this comment

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

one other note: the search block's width side panel still appeared, even when spacing.width is set to false in theme.json (I also added the following to the search block); not sure if that is an issue.

"spacing": {
			"width": true
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking the time to look over this!

I wanted to keep the scope of this PR narrow and contained to the Button block; I actually wasn't aware that the Search block had width settings! With the new theme.json property that this PR adds, we could look into adding support to the Search block in a follow-up feature. That way we can keep the testing steps and branch scope concise.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I am personally fine with either; I think making an issue that states width support is missing for the search block would be helpful without being too burdensome? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skorasaurus issue has been created! #42159 😄 Let me know if there's anything else I need to do here, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the default needs to be true, because this setting has been available to users and they may expect it to be available.
Personally, I would disable this option for some projects, and I welcome the possibility to do so, but I think that because it is an existing option, disabling it needs to be the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carolinan I agree, we definitely don't want this panel disappearing for people who expect it to be there and aren't staying on top of release notes. I've made some adjustments so that we expressly look for spacing.width === false when determining whether or not to display the panel.

@skorasaurus
Copy link
Member

Thanks again for putting this together - this is one of the most requested features for gutenberg - the width settings panel is not visible for me when I set spacing.width to true, as shown in my theme.json that I am using for testing. I am using a hybrid theme (classic php based theme with theme.json); I haven't tried with an FSE theme yet.

@carolinan
Copy link
Contributor

Same, I am unable to re-enable the width setting.

@roseg43 roseg43 force-pushed the add/button-block-width-theme-json-support branch from 300bc3a to aadb682 Compare July 7, 2022 15:27
@roseg43 roseg43 force-pushed the add/button-block-width-theme-json-support branch 2 times, most recently from 840d4e5 to afa8f2b Compare July 7, 2022 17:40
@roseg43 roseg43 requested a review from nerrad as a code owner July 7, 2022 17:40
@roseg43 roseg43 force-pushed the add/button-block-width-theme-json-support branch from afa8f2b to c03363f Compare July 7, 2022 18:06
@roseg43
Copy link
Contributor Author

roseg43 commented Jul 7, 2022

@skorasaurus @carolinan The spacing.width setting should be working properly now. I didn't notice that there was a property whitelist in lib/compat/wordpress-6.0/class-wp-theme-json-6-0.php where the new property needed to be added.

I noticed that after rebasing trunk onto the branch the Check local changes job is failing. Is there anything I can do to resolve this?

@roseg43 roseg43 force-pushed the add/button-block-width-theme-json-support branch from c03363f to 9fa6206 Compare July 7, 2022 18:29
@skorasaurus
Copy link
Member

skorasaurus commented Jul 7, 2022

Sorry, I don't know about the check local changes failure;

I tested the plugin using the zip that was generated at https://github.com/WordPress/gutenberg/actions/runs/2631572763 which should have all commits through 9fa6206 .

Unfortunately, with that, the width panel still appears when I had "width": false in my theme.json

This allows theme developers to manage support for the Width panel on the Button block using theme.json. We toggle visibility for the panel in Button's edit component using useSetting to grab the new setting. Appropriate entries have been added for the new setting to both the theme.json schema as well as the core-blocks and theme-json-living documentation.

Fixes WordPress#38767. See WordPress#19796
@roseg43 roseg43 force-pushed the add/button-block-width-theme-json-support branch from 9fa6206 to b01cbed Compare July 7, 2022 19:44
@roseg43
Copy link
Contributor Author

roseg43 commented Jul 7, 2022

@skorasaurus Found the issue. :) After pulling in the latest changes from trunk, I needed to add the new property to lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php. Could you try one more time, please?

@skorasaurus
Copy link
Member

skorasaurus commented Jul 7, 2022

@skorasaurus Found the issue. :) After pulling in the latest changes from trunk, I needed to add the new property to lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php. Could you try one more time, please?

Thanks for your persistence tracking that down.
after this most recent push - #42079 (comment)

Using either code snippets into theme.json, the width panel does not display in either instance as expected, hooray :)

    "blocks": {
      "core/button": {
        "spacing": {
          "width": false
        }
      }
    },

or

"spacing": {
  "padding": true,
"width": false
}

Which is great and what we intended!

Personally, I would not have the width available by default, to match the other spacing options (padding, margin) but I defer ultimately to @carolinan or someone else on core.

Testing this on classic themes (that do not have a theme.json file)

Came back to this; and my php parsing of JSON may be wrong; but the following function and hook should disable both gradients and the width panel in a button block; in a classic theme without theme.json (e.g. twentytwenty)

function remove_button_options( $metadata ) {
    if ( 'core/button' === $metadata['name'] ) {
	$metadata['supports']['spacing']['width'] = false;
 	$metadata['supports']['color']['gradients'] = false;
    }
    return $metadata;
}
add_filter( 'block_type_metadata', 'remove_button_options' );

In the above function and hook, the gradients are disabled as expected but the width panel still displays. :(

@roseg43
Copy link
Contributor Author

roseg43 commented Jul 27, 2022

Sorry @skorasaurus, this got away from me a bit ( client work getting in the way 😔 ) I'm going to take a look this week and see if I can't diagnose your issue.

I think it may have something to do with class-wp-theme-json.php in the main WordPress repo, but I'm not sure quite yet.

However, is this within scope of the PR? We're expressly targeting theme.json here, rather than functionality for toggling the Width panel visibility as a whole. If the fix for this ends up being fairly minor, I'm not against adding it, but we may want to consider a separate issue/PR for non-theme.json support.

@skorasaurus
Copy link
Member

skorasaurus commented Jul 27, 2022

However, is this within scope of the PR? We're expressly targeting theme.json here, rather than functionality for toggling the Width panel visibility as a whole. If the fix for this ends up being fairly minor, I'm not against adding it, but we may want to consider a separate issue/PR for non-theme.json support.

That is a good question; I don't speak on behalf of the core team (I'm not a core team member) and I don't know whether the PR (and other PRs) should necessarily include non-theme.json support. I do believe that the transition of adopting aspects of Gutenberg can be more difficult than what I anticipated and I'm even not completely 'sold' on theme.json although I currently use it in some (but not all) of our projects (and make the decision whether or not to use it).

If a decision is made for multiple PRs and for the future, it should be made should be very clearly and prominently articulated to users.

(as I'm sure you know; there's a spectrum of which aspects of Gutenberg that developers, organizations are adopting for a variety of reasons (lack of feature parity or established best practices, code changing too quickly; assumptions and architecture decisions that are baked into Gutenberg differ from 'pure' PHP; lack of time to train existing employees, migrate/update custom themes, plugins; developers are more experienced/comfortable with PHP than react; etc).

@scruffian
Copy link
Contributor

scruffian commented Jul 29, 2022

This is looking good. I'm not sure that width belongs under spacing though. Perhaps we need a new section called dimensions? I just saw that spacing is actually called dimensions in the UI so maybe not!

@roseg43
Copy link
Contributor Author

roseg43 commented Jul 29, 2022

This is looking good. I'm not sure that width belongs under spacing though. Perhaps we need a new section called dimensions? I just saw that spacing is actually called dimensions in the UI so maybe not!

Yep! Other properties that affect the box model show up in this section too, such as padding

@aaronrobertshaw
Copy link
Contributor

Thanks for working on this @roseg43 👍

It might be of interest to take a look at an old PR that was adding width block support as well as the follow up trialling that width support for the Button block.

I just saw that spacing is actually called dimensions in the UI so maybe not!

Initially, we were looking to unify all the spacing and dimensions settings under dimensions as that would reflect the panel they belonged to. We decided against that, however, and were going to have width and height support settings under dimensions separately rather than under spacing. Some history can be found in: #32392.

While we can migrate theme.json schemas to rename or move settings, I'd still be cautious about adding one that could conflict with or be confused with a proposed block support.

All other spacing and sizing related options are toggle-able via theme.json. As such, Button width settings should be consistent with its related options.

I disagree that Button width settings should be consistent here as they aren't a theme.json feature at this time. I do agree that a width (along with height) block support makes sense and could be implemented.

Theme.json settings are generally for block support features or presets. The width control for the Button block is an ad-hoc control used by the Button block and isn't rendered yet within the Dimensions panel. This makes me more inclined to believe it shouldn't belong wrapped up with the spacing block support settings.

I don't believe theme.json should support flags for all possible ad hoc controls rendered into our inspector controls sidebar.

cc/ @andrewserong @ramonjd @oandregal for additional insight, perspectives, and ideas.

@roseg43
Copy link
Contributor Author

roseg43 commented Aug 15, 2022

Theme.json settings are generally for block support features or presets. The width control for the Button block is an ad-hoc control used by the Button block and isn't rendered yet within the Dimensions panel.

@aaronrobertshaw Unless I'm misunderstanding, by this logic, shouldn't typography.dropCap not be present in theme.json since it only controls an ad-hoc core/paragraphblock option? There is a precedent for theme.json to contain options for block functionality that requires explicit theme support to work properly and can't be disabled by any other means.

I'd still be cautious about adding one that could conflict with or be confused with a proposed block support.

I agree that there is some potential for confusion in adding a property with a name as general as spacing.width. It could be assumed that this would also affect other blocks that allow for custom widths i.e. core/column. However, at this time, there's no top-level property that this would be more suited to.

I don't believe theme.json should support flags for all possible ad hoc controls rendered into our inspector controls sidebar.

I 100% agree with this, however I think the Why? of the PR covers why this specifically has a demonstrated need.

@ramonjd
Copy link
Member

ramonjd commented Aug 16, 2022

Unless I'm misunderstanding, by this logic, shouldn't typography.dropCap not be present in theme.json since it only controls an ad-hoc core/paragraphblock option?

dropCap is an historical curiosity. I can't exactly pinpoint the origin story, but it looks like it was pencilled in as an option for all text blocks. PR here.

For whatever reason it has only been implemented for paragraph blocks cc @jasmussen for fact check.

The theme.json control came later, see: #6184 (comment)

As Aaron hinted at above, I would have thought that "width", as part of settings.spacing in theme.json would have benefits beyond toggling a single block's control, namely to enable width control for a larger suite of blocks, for example the separator block.

The block supports framework has opt-in behaviour baked in.

Is there any scope to revive #32502? I think the effort would pay off and have a wider impact.

@andrewserong
Copy link
Contributor

Great discussion here, and nice work @roseg43 in working toward a solution.

Is there any scope to revive #32502? I think the effort would pay off and have a wider impact.

I think reviving exploring adding in a width block support feature that enables the control in other blocks would be a great idea. The use case of disabling width controls is important, but my concern with adding a settings.spacing.width flag to theme.json and its schema is that it signals that it's a feature that would also enable width controls on a wide array of blocks. Also, the predictability of setting settings.spacing.width to false is a challenge given that it doesn't affect the Column or Search blocks.

If we proceed with a value that disables existing controls, I think it'd be important to ensure that the value aligns with the proposed block support that ultimately enables the feature, to avoid the migration issue.

however I think the Why? of the PR covers why this specifically has a demonstrated need.

In the Why section, it describes themes having to explicitly add support for the classes output by the Buttons block. Is the use case here themes that have de-queued the Buttons styling that ships with the block? My understanding is that the block already provides the styling rules, so I thought themes wouldn't need to do anything explicitly to support width styles. Apologies if I'm missing something obvious, here! I'm keen to better understand the use case.

In terms of how we allow visibility controls for ad hoc controls, is there a comparable issue with other ad hoc controls such as minimum height of cover?

@jasmussen
Copy link
Contributor

For whatever reason it has only been implemented for paragraph blocks cc @jasmussen for fact check.

Dropcap is only implemented at the moment in Paragraph. I can't off the top of my head think of other blocks where it might be useful. But it's still nice for themes to be able to disable it, which I believe TT2 does.

@roseg43
Copy link
Contributor Author

roseg43 commented Aug 16, 2022

My understanding is that the block already provides the styling rules, so I thought themes wouldn't need to do anything explicitly to support width styles. Apologies if I'm missing something obvious, here! I'm keen to better understand the use case.

@andrewserong I could've clarified this a bit better, that's my mistake! #19796 discusses the need in greater detail, but Button is one of the only non-layout/atomic blocks that supplies width controls. As an atomic component, a button generally has fairly rigid style requirements within the context of a design library.

Unlike Columns, customizable button width is not an essential functional requirement of Button. It's a design decision, and one that developers should be able to dictate if it fits within the design parameters of their theme.

Currently, the only option that developers have to remove support for button widths is to dequeue the styles, leaving a set of controls that don't do anything. The result is a messy (and inaccessible) user experience.

Is there any scope to revive #32502? I think the effort would pay off and have a wider impact.

@ramonjd I love the ideas proposed in #32502 but that's a massive scope increase, the end result which looks like it might be in conflict with a larger design decision in the works re: layout blocks.

Overall, I understand the concerns about the naming of spacing.width and the assumptions it could create. I'm just not sure what an alternative looks like, as if a general block support for width is added, Button will likely use it, so having a property specifically for buttonWidth will introduce a property we have to migrate later.

After looking at the #25999 which added the width controls to Button, it seems like the conversation surrounding this feature didn't touch on any of the non-technical risks that come with making this a non-toggleable control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block
Projects
Status: PR needs review
Development

Successfully merging this pull request may close these issues.

Allow to disable settings for specific blocks
8 participants