Dimensions Panel: Add new ToolsPanel component and update spacing supports #32392
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: -7.48 kB (-1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
This is great work, and acting as expected in the editor and frontend. I know it's WIP so take everything with an industrial-sized silo of giant-leech-killing salt. I just had some observations, couched with all the "I'm no expert" and "I haven't been keeping up with background design/testing discussions" caveats. :) When I click "Reset all" I can see the effect in the editor (the margins disappear). The panel collapses and the controls disappear too. This is not the case on the story component demo where "Reset all" reinstates the controls.
I think this is just a personal expectation so don't read anything into it, but when I reset a value I anticipate seeing the result of the action, that is, the reset/cleared input value. Unchecking individual controls resets the value too, rather than toggling the UI only. I'm wondering how we can communicate this better. Toggling a check mark doesn't tell me what I'm about to do. I only have my experience to draw upon, so it may be totally obvious to others 😆 Personally it took me a bit of clicking around to move through the panels and have to work out what this new, "mystery panel" was doing.
Would it help accessibility if the entire body panel title toggled the ellipsis menu?
Sorry for all the questions, I hope I'm not repeating what's already been addressed. Thanks! |
All feedback and testing is appreciated, thank you! 🙇
This is intended however I'm certainly open to better approaches. The idea here was that the user is opting in or out of a specific feature not just toggling the display of a control. In this case, to opt out of a block support feature it needs to be reset.
The trial typography panel in the G2 storybook maintains its own store and resets to an arbitrary default state that contains values for the controls. If the controls in this new panel have values they are shown. So the difference here is really what we are resetting to. In the demo an arbitrary state with values. In this PR with block supports, the block's attributes for the block support are undefined by default and hence don't show.
This is a fair expectation. I'm not sure how we could communicate what is happening better when individual features are reset/toggled off. Perhaps the menu group label of "Display options" should actually reflect the purpose in this use case i.e. which block support feature is enabled.
Good point. If we can make the menu as a whole clearer, that the options are opting in/out of a block support feature not toggling UI, perhaps the check mark also becomes clearer.
I wonder if this would be easier if all or the majority of panels were switched to the progressive display approach?
This sounds like an improvement to me although I'm not sure what issues that might present in terms of layout/positioning. Ultimately, for this first iteration I avoided going down that path, particularly given the demo G2 typography panel only relied on the ellipsis button itself to toggle.
Thanks again for the thorough feedback! I think most of the issues raised can be boiled down to a difference of expectation i.e. opting in/out of a feature versus toggling display of a control on/off. Hopefully with a few tweaks we can improve the UX. @jasmussen it would be great to get your thoughts and suggestions on this PR and some of these issues if you can spare some time 🙏 |
|
This is potent! Thanks so much for working on this. It seems a small step, but it's such a key step. Here's what I see:
Even in this state, it's working incredibly well, and almost exactly as I'd hoped it would:
One effect of this system surfaces an aspect not explicitly covered in the original ticket: the idea of default values. For example in a "Typography" panel shown in the block editor, I would expect "Font size" to be shown by default, and perhaps a future "Style preset" picker, but nothing else. If you need line height, letter spacing, letter case, any of those options, you would need to dig into the overflow menu to add them. However when you are in the Global Styles interface, we might want to surface all controls available at all times for all blocks, because that's where you set defaults. In other words, the missing bit here is a "defaults" system. And since there's only "Padding" available for the Group block, at least for the foreseeable future I'd show it by default. This is not a knock on your work here at all — I expect additional dimension controls to land here needing to be explicitly added — it's just that the full potential probably isn't shown until we employ it for the Typography panel. So how would a defaults system work with the "More" menu? Like so, I would expect:
Does that make sense? |
|
Great feedback as always @jasmussen, thank you. Glad this is in the ball park for what you imagined 👍
It would certainly round out the UX. The current approach uses a callback on the block support feature's control component to determine if it has a value and therefore should be shown. A tweak to the intent of that callback would nearly have us there. It might fall down though when the context is different e.g. the block editor vs site editor's Global Styles. Perhaps a simple prop on each control added to the panel could flag if it is to be displayed by default. When the panel generates the menu it can collect which are shown by default and use that when resetting. Under global styles they can all be set to true, and set as desired when in the block editor.
Yes. When the UI for the block support is created, each block support feature's control is conditionally added as a child to the panel. It's from those children the menu is created. At the moment only the padding, and margin support if following testing instructions, would show in the dimensions panel. The typography panel was the next target if this PR showed promise. The panel can be styled to be a flex or grid container as well so the typography panel is a good chance to see how the controls can be laid out as they get toggled on and off.
That all sounds good. My only question would be do you see different block types needing to have different defaults for display? What I mean by that is, take two block's, they both opt into the same block support feature e.g. font size. Would it be safe to say if font size was to be displayed by default it should be displayed by default for all block's opting into it? Or, could we have a situation where it should be shown by default for one block but not another? I'll add an initial means of handling default display. Something to be able to be seen and played with at least. |
That is an excellent question, and I think we maybe want that. You should almost never change the font in a single paragraph, but you might want to in a pullquote. Or a third party building a block might want to suggest it by default. If per-block defaults (as opposed to per-panel defaults) is something we think we can add in the future, I think it's fine to not worry too much about that use case at the moment. The more important use case is the differentiation between block and global styles. |
|
@jasmussen I've had a go at adding default controls to this panel. It's pretty close though I don't think it's perfect. There is just a small issue when you have default controls. They still need to be in the "more" menu so they can be reset. The menu item for these controls gets updated to have a check mark beside them when their value is set. The problem comes when the control is empty and the user opens the menu. They see an unchecked menu item for the control. When that is clicked, nothing visually happens. They can then toggle it again to uncheck it and since it was empty it still appears that nothing happens.
Do you think it would be ok to perhaps disable a default control's menu item when it doesn't have a value set? Without a value there is no need to reset the control via the menu. Disabling it under those circumstances might prevent confusion as well as convey that the user doesn't get the choice as to whether that control is visible or not? |
|
Wonderful. Yep, we're down to the key remaining issue; the checkmark in the more menu serving as both a way to reset the control, and as a way to remove the control if it's not among the list of defaults. I still think that behavior is key to keep, though, at the very least as a baseline we can learn from. Let's imagine a Typography panel in the Paragraph that has the following:
Of those, only the Font-size is surfaced by default. So you click a paragraph, and it's present, even if it is unset. You can toggle it off which both removes the control, and unsets any value it might have had, or you can add it back. In Global styles, all those controls would be surfaced by default. Here, you would also be able to toggle them off to reset any user customizations you may have made, and if you added a control back it would revert to the theme.json provided styles, or be unset, which ever is the case. In the global styles case, any controls you toggle off would also be back if you reloaded the page, even if those controls are unset. So, how about this:
Does that make sense? |
|
Great feedback, thanks @jasmussen! I've updated the default controls to have a check mark beside them by default and also get removed when toggled off. This brings the panel's behaviour inline with the last description of requirements. It would be good to confirm though how the "reset all" works. Given that default controls can now be toggled off for display I think it makes sense that the behaviour is consistent between them and non-default controls. By that I mean, when resetting all controls, non-default controls get reset and toggled off. The current state of this PR is that when "reset all" is selected in the "more" menu, all controls are reset and removed from the panel. If you select a different block, then reselect the original block, you'll get the default controls displaying again. If you are happy with the new functionality, this should be getting pretty close to just needing a final review. |
|
Thanks again so much for working on this, it really is quite crucial. By the way I'm seeing margin and padding hidden by default for the group; since there are so few options there, we might want to just show those by default. Before I dive in I wanted to note that I suspect something like the typography panel will be an opportunity to refine the precise behavior of this system, and address any shortcomings there might be. So, right now in this branch, unchecking a control:
That behavior appears to be the same as that of the original prototype, which works like this:
In this prototype, "Reset all" does two things:
How would that translate to the Paragraph block, which showed only font-size by default? We can think of it as a "factory reset": if invoked on a Paragraph that has font, size, case and line-height customized, all values would be cleared and you'd only see the an empty font size control remain. So factory reset would clear all values, and reset the configuration of controls to show the defaults again. I acknowledge that this might potentially be confusing since the checkmarks also serve as reset buttons, but I also think in practice it might feel just fine. What do you think? |
I wasn’t making any calls on which blocks should enable which controls by default in this PR. Under the “Testing Setup” section in the PR description I added instructions on configuring the default controls.
I'm assuming "this prototype" meant the original G2 prototype linked not this PR's? This PR's approach takes a It then toggles all controls off in the menu, thereby preventing them from being displayed, default control or not. That is, until the block is deselected, then selected again at which point the default controls would appear.
That isn’t the case at the moment. You reset all, they all get reset and toggled off. This made the "reset all" option more consistent with the individual items which reset the value and toggle off a control.
This sounds much like the situation before the last round of changes only that now we can temporarily remove a default control. We end up with a different problem than that of the previous iteration. Instead of seemingly toggling a control that does nothing. Now resetting doesn’t get rid of a control so its behaviour is different than the individual menu items in terms of what it does to display of controls. I can change the current approach of course. We only need to decide on how it should behave.
I’m not 100% sold on the check marks and menu items being dual purpose. It feels like either way we go there is obscure behaviour. I think visually the panel UI looks much better, though functionally, it's less clear what does what, that additional controls even exist etc. Ultimately, if this is what is needed to provide expanded block functionality to users, it might just be the cost of doing so in the short term. Thanks again for the feedback. This wouldn't be progressing without it 🙇 |
Ah, my bad, I got excited 😄
Yes, sorry, this prototype: https://g2-components.xyz/?path=/story/designtools-presentation-typographypanel--default
Thanks for your patience and clarity. Yes, I personally think it's acceptable that the "reset all" toggle removes only some of the controls, so long as it also clears all of them. But more importantly, I'd be happy to try the simplest path forward, provided we believe we can go back and refine this. In other words, if we don't paint ourselves into a corner with a decision point here, it seems okay to try one thing and then revisit after, for example, trying it in the typography panel. In this case, a reset all that just factory resets the panel — clears all values and corresponds to deselecting and re-selecting the block to reboot the defaults — is the best path forward.
I think it's easy to get caught up in the intricacies of the behavior, and yes we definitely want to refine it. But it's also worth zooming out and walking through the flow and the problem we are solving:
Defaults, in this case, are key: by allowing us to configure a default selection of controls on a per-block basis, we can start to create a slew of amazing new design tools that immediately become available across blocks, without weighing down the UI. As we move forward we can then fine-tune those defaults to get it just right, all the while enabling design tool work. In practice I expect clearing values or "reset all" to be very rarely used tools; Figma doesn't even have them: if you want to clear a line-height, you select the value and delete it. So clear-tools have intentionally had their prominence reduced to match their value. |
That's a good thing! It should mean we're making progress 🎉 I just imagined that we'd need a methodical approach to go over every block opting into each set of support features and deciding upon the appropriate defaults. A separate PR/s would help separate those discussions from how this panel worked in general.
It's been a team effort! I appreciate all of your guidance.
I can make the change such that the "reset all" will not prevent a default control from displaying. My plan would be:
If we settle for all controls being removed when "reset all" is selected, nothing further needs to be done. Either way, we can change to the alternate approach easy enough and definitely won't box ourselves into a corner. Given it sounds like the first approach is the preferred option, I'm happy to make that change. Then once this PR lands, we can put the new panel through its paces updating the Typography panel.
It is easy to not see the forest for the trees sometimes 🌳 For what it's worth, I am 100% sold on why we are reimagining the UI and what we're trying to achieve 🙂 |
Sounds like a plan, I think it's worth trying it, especially if it's easyish to tune! Thank you.
💯 |
|
The changes to the reset all functionality have been made. After the reset all menu item is selected all the default controls will be shown.
@jasmussen do you see anything else we need to improve from a UX perspective here or do we just need to get final approval on this PR and start using it for other block supports? |
|
From what I can glean in the conversation about this behavior, it seems like it's just about ready to merge. Though I wasn't able to fully test the defaults — my experimental-theme.json file is likely very out of date — are you able to provide a more complete theme.json example where margin and padding are both enabled by default? That also leads me to my last question: these "shown by default" settings, are they per-panel, or per-block? Ideally they are per-block. For example in a future typography channel, we might only want to show font-size by default for the paragraph, but we might want to show both font-size, line-height and letter-case on the Pullquote block. |
| @@ -88,9 +110,9 @@ function gutenberg_skip_spacing_serialization( $block_type ) { | |||
|
|
|||
| // Register the block support. | |||
| WP_Block_Supports::get_instance()->register( | |||
| 'spacing', | |||
| 'dimensions', | |||
There was a problem hiding this comment.
This is the sort of thing I brought up at https://github.com/WordPress/gutenberg/pull/32499/files#r686747072
Isn't this problematic with existing spacing supports? What happens when core (WordPress 5.8) registers support for spacing and the plugin does not overwrite it as before but instead registers a different thing called dimensions?
There was a problem hiding this comment.
It might make more sense to keep things anchored to spacing for now.
There was a problem hiding this comment.
Thanks for the explanation @nosolosw, I think I understand my error now in not appreciating that core also register support under the same key.
I'll create a quick PR to change the key above from dimensions back to spacing and add an inline comment explaining why it is still spacing.
Regarding the comments on the PR introducing height block support I think there may be some confusion there, at least there is from my side, so I'll reply in more detail on that PR.
There was a problem hiding this comment.
Fix is up in: #34030
Thank you for catching this one!
There was a problem hiding this comment.
Thanks for the quick turnaround, as always!
| function gutenberg_register_spacing_support( $block_type ) { | ||
| $has_spacing_support = gutenberg_block_has_support( $block_type, array( 'spacing' ), false ); | ||
|
|
||
| function gutenberg_register_dimensions_support( $block_type ) { |
There was a problem hiding this comment.
I have a second thought about this PR: I'm unsure whether we can rename a function that's been already shipped to WordPress core (see). Same for renaming the file: spacing.php to dimensions.php. These become "public APIs" once shipped and, generally, we can't remove them without notice: there's a deprecation process if need be, etc.
Would like thoughts from folks with more experience working with WordPress core: @gziolo @jorgefilipecosta @youknowriad
There was a problem hiding this comment.
I agree, it's similar to the above comment, even if this functions is marked "internal" in Core, so in theory we could rename, I think the file should be kept as spacing.php and the function renamed... Just stay consistent as long as the API is "spacing".
There was a problem hiding this comment.
I'm happy to rename this file back to spacing.php, restore the function name, and can do this tomorrow.
I was under the impression we wanted to collect all the current spacing and proposed dimensions support under a "dimensions" toolset. Hence, the rename and combining of both here, my apologies it's missed the mark.
Would it be better to revert this file back to the original spacing.php and create a new dimensions.php that will contain the height and width supports?
The UI created via dimensions.js for the block editor and dimensions-panel.js for site editor should be able to stay mostly as they are, correct?
There was a problem hiding this comment.
I'd be in the camp of using "Dimensions" for the UI labels... but keeping "spacing" for the code because of the backward compatibility concerns.
There was a problem hiding this comment.
Nothing to apologize for, that's what reviews are for!
Judging by the issues you linked, at the user/UI level, I presume the intention is to keep everything together, as you already did.
At the developer/code level, these are the boundaries that I see:
- Keys in
block.json&theme.json: as long as we use the same structure in both places, we should be fine. Either keeping everything under thespacingnamespace or usingspacingfor the existing (margin, padding) anddimensionsfor the new (height, width). - Server hook: either having a single hook (
spacing) or two (spacinganddimensions) should be fine as well, following what we decide for the keys. - Client hook: keeping a single UI hook to gather all props together looks simpler than trying to use SlotFills for this. Unlike the server code, we don't export the client hooks or the functions affected by this change, so keep the name "dimensions" instead of "spacing" is a style choice — it also matches well with what we use at the user level.
Within these boundaries, I believe the choices would be up to you. I can review that PR to help consolidate this code.
|
Hey @aaronrobertshaw , sorry if this is being tracked elsewhere, but is there any progress on the conversion to TypeScript for the |
|
Hi @ciampo, a teammate started the conversion but we got caught up on other tasks. It will likely be a couple of days until I can revisit it. The draft PR can be found here: #34028 cc/ @glendaviesnz |
|
Thank you @aaronrobertshaw , I'll keep an eye on it! |
|
I'm not sure how related exactly but I noticed that when moving the mouse (and doing nothing else), the ToolPanel components are constantly rerendering (seeing them highlighted using the React dev tools). This might potentially have some performance implications (probably not important for most but accumulation can make these things a problem) |
|
Thanks for raising this @youknowriad 👍 I believe this is actually an issue with the When I hover and move the mouse around over other controls in other There are open issues and PRs aimed at improving/fixing how the box control's visualizer data is handled.
GIF of what I see
If I've misunderstood the issue you were reporting please let me know. |
|
I think it's a different one, see now I have a group block selected and I move the mouse over the editor canvas, the tools panels keep flashing Screen.Recording.2022-01-12.at.9.46.55.AM.mov |
|
@aaronrobertshaw Here's what I found so far:
|
|
Thanks for the extra details @youknowriad. I can see what you mean now. Your explanation also covers why I didn't notice it when I tried to further isolate the issue by looking at the component within the storybook. I've tried adding memoization to the Unfortunately, I'm out of time today but will pick this up again on Monday. |
|
The updates to the |
Part of: #28356
Description
ToolsPanelcomponentsThe new panel component offers progressive discovery options for
ToolsPanelItemwrapped components e.g. those provided through block supports.It uses callbacks passed as props on the panel's children to check for values, handle child selection/deselection in the menu etc. In the case of block support controls, the
onDeselectcallback can reset the block attribute value.The
ToolsPanelautomatically generates its menu from children that register themselves via the panel's context. The initial state managing which controls are displayed and therefore selected in the menu is based off thehasValuecallback and anisShownByDefaultprop.A custom
resetAllcallback is passed to the panel and called when the associated menu item is selected. For the a panel displaying block support controls such as the dimensions panel, theresetAllcallback will reset all block attributes for the dimensions related support.G2 Components
The demo typography panel from the G2 project still needs a few underlying components and systems to be imported into Gutenberg. The approach in this PR works now and may unblock other block supports work.
New designs/G2 components for the individual block support controls can be iterated on separately.
How has this been tested?
Programmatic tests are available for the new panel:
npm run test-unit packages/components/src/tools-panel/test/Storybook:
npm run storybook:devManual testing is required for the updates switching the spacing block support UI to the new panel and "Dimensions" naming.
Testing Setup
The easiest block to test with is the group block.
settings.spacing.customMarginunder your theme.json file.block.jsonand turn on margin support undersupports.spacing.__experimentalDefaultControlsflag undersettings.spacingin block.json to specify which block support controls are displayed by default.The example below configures only the padding control to show by default.
Testing Instructions
The following instructions assume the padding control was enabled by default as per the snippet above.
Screenshots
With Default Controls
Without Default Controls
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).