Dimensions Support: Add SlotFill to Dimensions Panel #34063
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Size Change: +106 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
I've been thinking about this a bit and right now for controls in the toolbar, we do something like: This puts the added control into the "inline" section of the block controls. I wonder if we could be able to have a very similar API for the block authors. This would make adoption and devx better. I know @gziolo has also thought about this previously. |
|
@youknowriad you read my mind. It's something I was talking about with @jasmussen in the context of absorbing the parent inspector controls as a follow-up for #33955. My initial idea was to have the following groups:
Now, there is a question about what we do about Global Styles UI. If that should be a single group or we should have more groups, for example:
|
|
I started #34069 to explore groups for the inspector controls slot. |
|
I think Slots for Inspectors need to be aware of the normalized panels we're introducing (dimensions, typography...) like this PR does, and yeah ideally also handle the menu to hide/show and reset these controls automatically. |
aaronrobertshaw
force-pushed
the
try/slot-fill-for-dimensions-panel
branch
from
ab8a037
to
6c0813e
Compare
|
Thanks @gziolo for putting together the grouped I've rebased this PR onto that one, added a Are there any concerns over the general approach to providing the grouped |
aaronrobertshaw
changed the base branch from
trunk
to
update/inspector-controls-groups
| @@ -6,11 +6,15 @@ import { createSlotFill } from '@wordpress/components'; | |||
| const InspectorControlsDefault = createSlotFill( 'InspectorControls' ); | |||
| const InspectorControlsBlock = createSlotFill( 'InspectorControlsBlock' ); | |||
| const InspectorControlsAdvanced = createSlotFill( 'InspectorAdvancedControls' ); | |||
| const InspectorControlsDimensions = createSlotFill( | |||
| 'InspectorDimensionsControls' | |||
| ); | |||
|
|
|||
| const groups = { | |||
| default: InspectorControlsDefault, | |||
| block: InspectorControlsBlock, | |||
There was a problem hiding this comment.
I'm not sure "block" group for the inspector controls make sense.
There was a problem hiding this comment.
It comes from the parent PR that I opened. The reasoning is that block would contain all controls specific only to the particular block. Image is a great example here:
I also don't feel strong about that and we can use default for those controls as well.
|
I think this is going in the right direction. Looking at the example usage in #34065 I have a few questions/notes. Basically the API is something like this: Now correct me if I'm wrong but here are my assumptions:
I also feel we should add some documentation and update existing examples in our markdowns to rely on these APIs (could be follow-ups) |
|
I was also wondering about the naming of |
My understanding or assumption was that the slot was just a place for us to insert whatever was needed into the block support’s panel. Are you advocating that we move the dimensions slot to inside the InspectorControls then have it create the I’m not clear on how that would work. It feels like it's complicating how the block support hooks generate their panels further. Is there a way we can address the API and devx concerns while keeping this clearer? Would it be better to keep all that block support related functionality within the appropriate block support’s hook?
We currently have the ability to add whatever we want within a
At this stage, we wouldn’t have any backwards compatibility concerns around the use of the slot for block support panels. Are you seeing that we might in future and we want to restrict the type of items that can be added via the slot?
Definitely. I’m happy to add the new groups and update the other block supports as well as the docs once we settle on an approach here.
There was a bit of discussion around the naming of the If you have new suggestions, the sooner we rename it the better. No better options have gained any traction to date though. |
I'm actually advocating for making The controls should stay in the hooks but should use This maps exactly how |
|
To be honest my suggestion doesn't change much, it just makes it easier to add new Slots like that. "dimension" is nothing more than a new "group". Basically the only difference between both is whether we do this: or just and have the Slot component render the ToolsPanel automatically like we do for BlockControls. I don't feel that strongly about it though. So your call. |
|
This might just be a style preference, but with the question about where to place the That sounds good to me, so I think I'd lean toward how it's implemented in this PR. However, I also see an argument for generalising so that the concept of extending the panel is more about the panel rather than intrinsically extending the concept of the particular block support. Would it work to go with the current approach and revisit in a follow-up if it looks like we need to make it more generic? |
Ok, this sounds like what I was trying to convey in my question. That panel has to be created in the slot but only for certain groups. The group's slot then has to be added to the InspectorControls somewhere. If that isn't via the dimensions support's hook, then we need to move it to the InspectorControls' fill itself correct?
Understood.
My confusion is over how the To be able to perform a "reset all" for these group slots' panels, I imagine we could get the currently selected block from the store and create a generic I feel like it would be good though for individual block supports to be able to control that behaviour themselves. Maybe resetting focus to a specific control after reset or something. Block support provided styles have already overlapped some block styles, so could there be the possibility that the reset all function might want to restore a block style selection? Perhaps, with the image block the user selects the rounded block style, then decides to adjust the border-radius explicitly. They decide to reset the explicit radius. Could we down the road want to restore the block style selection there to rounded? The additional handling for retrieving block props, that are already available within the block support hook, creating the callbacks etc seem to make this diverge from the example of the As you elude to, the normal use of this from a third-party dev's perspective doesn't really change. My view when first approaching this was for the SlotFill to provide a means of adding to a block supports panel rather than replace it. The little bit of extra duplication seems like a small trade-off for increased flexibility for each of the block supports. |
|
Thanks for adding to the discussion @andrewserong, you make some good points 👍
Interesting point. Having consistency around the order of at least the block support "built in" controls sounds important.
This is the key question. Do we change block supports to not provide their own panel but only add to one provided by the inspector controls?
This would allow us to move forward. The change being discussed doesn't alter how new controls are added so I don't think there's any blocker to iterating on it in the near future if needed. |
I'm not sure I agree with this, I think all items including the hooks one should be added via the slot. There are filter priorities to handle order if needed. The reason is simple, in previous occasions we had situations where third-party items want to force their controls to be top in the list (cc @retrofox as he refactored the "styles" in the inspector controls to do exactly that). I wouldn't be surprised if a core block needs this personally. |
Correct. It's a pity we haven't merged the PR, yet. 🤦 My bad |
Thanks for clarifying @youknowriad, that makes good sense to me 👍 |
gziolo
force-pushed
the
update/inspector-controls-groups
branch
from
435415b
to
e3309e8
Compare
|
Just letting you know that applied some breaking changes to #34069 and therefore to the base branch used here. The only important change is that |
This will allow for ad hoc controls to be injected by blocks into the dimensions block support panel.
aaronrobertshaw
force-pushed
the
try/slot-fill-for-dimensions-panel
branch
from
6c0813e
to
cc7e089
Compare
aaronrobertshaw
force-pushed
the
try/slot-fill-for-dimensions-panel
branch
from
cc7e089
to
3386a7d
Compare
|
Closing in favour of #34157 |
|
I will add a comment here. "The current downsides are twofold. Once choosing to display margin or padding controls, the panel itself cannot be collapsed. This exacerbates the very problem that the new feature attempts to solve — decluttering the sidebar interface. For me, at least, I always want quick access to spacing controls. However, I do not always need them shown." I like to close the panels as it declutters the sidebar. Which means not replacing the open/close arrow with the 3 dot contextual menu, but instead adding the contextual menu inside the panel. |
|
The new tools panel is due for polish in the 5.9 phase. One thing we could look at next is to remember controls you've added or removed from the ellipsis menu, so you can tailor your own experience. We can also revisit the decision to have the checkmark singularly clear and remove. Together these could address the need for occasional-but-not-always spacing controls. The tools panel will coexist with the collapsible panel. It's primarily meant to replace inspector panels that today start out opened: the ones that even if you close them to reduce noise in the moment are reopened as soon as you deselect and select the block again. Simplifying and reducing noise in the inspector is a high priority, and the primary goal for #27331 to solve. The tools panel is only one ingredient. Simplified color swatches, consistent reused panels, and simplified heuristics for control layouts are key aspects that still need to land. |
Related:
Description
DimensionControlsToolsPanelItemwas updated to allow passing a new function through to theToolsPanelthat will allow for continued filtering when passed as an argument to theToolsPanel'sresetAllcallback.Note: This is a very rough proof of concept. If anyone has more elegant solutions, I'd be happy to implement them.
Also, I've included a readme for
DimensionControlshowever I'm expecting we'll remove that given this would be experimental. For the time being it helps further explain the intent of this PRExample Usage
An example of the intended usage of this change is to be able to merge the ad hoc min-height control from the Cover block into the Dimensions panel provided via block supports as they currently share the same title.
How has this been tested?
npm run test-unit:watch packages/components/src/tools-panel/testTypes of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).