InspectorControls: Wrap block support slots in ToolsPanel #34157
Conversation
aaronrobertshaw
added
[Feature] Block API
gziolo
force-pushed
the
update/inspector-controls-groups
branch
from
435415b
to
e3309e8
Compare
|
Size Change: +156 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
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 |
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
b1b16a2
to
7c23fd8
Compare
This approach will make the block support hooks a little cleaner, reducing some boilerplate code. For the immediate uses planned, I like this approach as well. If a scenario does arise in the future where a block support panel needs to do something more than just reset attributes, we can cross that bridge then. While creating this PR I did run into some odd behaviour resetting the block attributes via I'll close the original PR in favour of this one. |
There was a problem hiding this comment.
This is testing well for me @aaronrobertshaw, I like this approach, and nice work fixing up the resetAll behaviour!
I was wondering if it's worth us adding a constants.js file to limit using string literals in the __experimentalGroup prop for dimensions, border, color, typography etc? It's definitely something we could look into in a follow-up instead (so not blocking for this PR), but just to help avoid accidental typos in subsequent PRs, which are sometimes tricky to debug. This comment might be more applicable to #34069 of course, so I'll add a comment over there, too.
Otherwise, looks great to me!
packages/block-editor/src/components/inspector-controls/slot.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js
Outdated
Show resolved
Hide resolved
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
gziolo
force-pushed
the
update/inspector-controls-groups
branch
from
83bc87e
to
8312f54
Compare
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
515f33d
to
852267f
Compare
packages/block-editor/src/components/inspector-controls/groups.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Good work @aaronrobertshaw, this is testing well for me, and nice idea using the panelId to ensure that injected ToolsPanelItems are connected to the correct ToolsPanel. It works well in testing, and as we discussed in #34065 (comment), once the __experimentalGroup prop stabilises, it'd be good for us to update the InspectorControls documentation with examples on how to use the ToolsPanel item and hook it up to the right ToolsPanel via the clientId. Since this is still experimental, I think it's fine to delay getting the docs in as we're likely to change implementation details over time.
The behaviour in this PR and #34065 feels solid to me in testing, and I think it includes some good pragmatic workarounds for the edge cases we encountered, so I'm giving it the ✅, but let's see what others think about the panelId idea, too.
|
@gziolo or @youknowriad do you have any other thoughts or concerns I need to address on this PR? It would be great to sneak it, and the related cover block PR, into this release if possible. In particular, I thought you might have some better ideas around how I solved the issues where switching between block selections still had previous fills hanging around which caused incorrect panel items to be registered. |
packages/block-editor/src/components/inspector-controls/slot.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Sorry for the late review, but I've only just realised this PR was in the works!
In general, it'd be good to keep the components squad in the loop every time a @wordpress/components component gets edited — and especially when the APIs are modified.
Also, it'd be great if in the future we could avoid mixing block editor and @wordpress/components changes within the same PR
aaronrobertshaw
added
the
[Feature] UI Components
ciampo
moved this from In progress (owned) ⏳
to In progress ⏳ (un-owned)
in WordPress Components
It's very much appreciated. My apologies for missing the components label on this PR and specific components squad members from the reviewers.
Understood. The changes here evolved from issues uncovered when attempting to implement the use of the ToolsPanel around the new grouped InspectorControls slots. These were also in a separate branch when this PR was first created, so splintering this again at that stage was problematic. With the majority of those issues debugged and addressed. I'd be happy to split out the ToolsPanel updates from this PR if it will help move this all forward. I'll also be more aware of isolating component changes moving forward. Thanks. |
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
f058696
to
574c27b
Compare
aaronrobertshaw
changed the base branch from
trunk
to
update/tools-panel-for-slotfills-use
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
574c27b
to
e9bbbed
Compare
aaronrobertshaw
removed
the
[Feature] UI Components
I'm afraid it isn't a realistic expectation in the project that has over 800 contributors. There isn't even a formal WordPress team for
Can you expand on this point? I don't think I agree on principle with this limitation, but many I don't know the full story. |
|
It looks like we need the following steps to move forward with this PR:
|
There are a few reasons behind my previous suggestion about keeping changes to the Firstly, splitting PRs into smaller chunks makes them easier to review and, in particular, isolating changes to the But most importantly, the The With this perspective in mind, I don't see this being a limitation — rather an improvement in how we collaborate on the This should hopefully also give more context on the other point about involving the components squad:
I agree that this is not realistic, and I quite like some of the ideas that you suggested. Ultimately we need to think about what's the best and most scalable solution. |
It depends on the perspective. From the development perspective it's easier to operate on a single branch as you can better drive the API decision which you can see in the discussions that happened in this PR. Splitting the work into smaller PRs to land changes seems like a good middle ground here if that helps to review the changes to
100% agree. This is exactly how this collection of components was born and evolved. That's why G2 components project has been started to address all the pain points you outlined and I personally love all the efforts towards making APIs unified and better organized 😄 |
Co-Authored-By: Greg Ziółkowski <grzegorz@gziolo.pl>
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
e9bbbed
to
269b5d1
Compare
|
Just gave this is another smoke test, and it's working nicely for me with the |
Depends on:
Related:
Dimensions Support: Add SlotFill to Dimensions Panel #34063Description
InspectorControls.resetAllfunction, getting block attributes from the store etc.How has this been tested?
Screenshots
Types of changes
New feature. Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).