-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: trunk
Are you sure you want to change the base?
Button: Add theme.json support for toggling Width settings panel #42079
Conversation
@@ -219,6 +219,11 @@ | |||
"type": "boolean", | |||
"default": false | |||
}, | |||
"width": { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
},
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
Same, I am unable to re-enable the width setting. |
300bc3a
to
aadb682
Compare
840d4e5
to
afa8f2b
Compare
afa8f2b
to
c03363f
Compare
@skorasaurus @carolinan The I noticed that after rebasing |
c03363f
to
9fa6206
Compare
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
9fa6206
to
b01cbed
Compare
@skorasaurus Found the issue. :) After pulling in the latest changes from |
Thanks for your persistence tracking that down. Using either code snippets into theme.json, the width panel does not display in either instance as expected, hooray :)
or
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)
In the above function and hook, the gradients are disabled as expected but the width panel still displays. :( |
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 |
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). |
This is looking good. |
Yep! Other properties that affect the box model show up in this section too, such as |
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.
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 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.
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. |
@aaronrobertshaw Unless I'm misunderstanding, by this logic, shouldn't
I agree that there is some potential for confusion in adding a property with a name as general as
I 100% agree with this, however I think the Why? of the PR covers why this specifically has a demonstrated need. |
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 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. |
Great discussion here, and nice work @roseg43 in working toward a solution.
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 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.
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? |
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. |
@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.
@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 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. |
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 totheme.json
. Appropriate entries have been added for the new setting to both thetheme.json
schema as well as thecore-blocks
andtheme-json-living
documentation.Support for this feature can now be toggled via the
spacing.width
property intheme.json
Testing Instructions
theme.json
for the currently active theme, addspacing.width: false
to thesettings.spacing
propertyspacing.width: false
is set at the block setting level intheme.json