Block Supports: Switch dimensions inspector controls slot to bubble virtually #34725
Conversation
aaronrobertshaw
added
[Feature] Block API
|
Size Change: +5.47 kB (+1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
from
94115c2
to
94f444c
Compare
| @@ -10,7 +10,7 @@ import { useDispatch, useSelect } from '@wordpress/data'; | |||
| import { store as blockEditorStore } from '../../store'; | |||
| import { cleanEmptyObject } from '../../hooks/utils'; | |||
|
|
|||
| export default function BlockSupportToolsPanel( { children, label, header } ) { | |||
| export default function BlockSupportToolsPanel( { children, label } ) { | |||
There was a problem hiding this comment.
The header prop is no longer used. I missed this when separating the ToolsPanel changes from those adding the ToolsPanel and Dimensions slot in #34157
|
Reverted the earlier attempt to move the ToolsPanel inside the slot since of course it would create a new panel for each set of fills. 🤦♂️ |
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Here's what I'm seeing:
I tested the following: ✅ Padding support controls work as expected in the block editor and frontend when enabling it at the theme.json block level |
There was a problem hiding this comment.
This tests well for me.
I don't have any specific comments on the approach, other than it does fix the bug in #34766 and the controls appear and function as expected.
Looks good in Chrome, FF and Safari.
I think it might need a quick rebase. There seems to have been updates to npm packages.
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
from
21e6769
to
090c908
Compare
|
I've rebased this and given it another test. Through that, I've found another side effect for the changes in this PR. The order of the controls added to the slot changes as they are toggled on and off. This isn't ideal but could be addressed separately in a follow up so that the fix for block specific settings lands.
There has also been talk of allowing consumers to control the order of the controls injected into these panels. Could this be addressed as part of a broader effort to add that? |
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
2 times, most recently
from
761f2c9
to
959ed4b
Compare
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
from
959ed4b
to
6abb295
Compare
| @@ -31,7 +32,12 @@ export default function InspectorControlsSlot( { | |||
| if ( label ) { | |||
| return ( | |||
| <BlockSupportToolsPanel group={ group } label={ label }> | |||
| <Slot { ...props } bubblesVirtually={ bubblesVirtually } /> | |||
| <BlockSupportSlotContainer | |||
There was a problem hiding this comment.
Does BlockSupportSlotContainer exist only to collocate the context hook with the Slot?
There was a problem hiding this comment.
At this point in time that's correct. I didn't find a better option given the tools panel context isn't created until the ToolsPanel is within the BlockSupportToolsPanel component.
With the current approach, there is also an issue with maintaining the order of rendered items in the panel when they are injected from multiple places. I'm definitely open to suggestions on improving this.
Another option I was planning to explore but expect some issues with was, to see if I could set up ref forwarding for the BlockSupportToolsPanel and pass it through the as prop to the virtual bubbling Slot. The idea being that the BlockSupportToolsPanel then passes the slot's ref on to a wrapping div and then creates the ToolsPanel with the fills inside that. The goal being the slot no longer introduces a div inside the ToolsPanel and there would no longer be a need to manually pass the ToolsPanelContext through the slot.
As I mentioned, I'm probably missing something with that idea. Let me know if there are any fatal flaws with it.
There was a problem hiding this comment.
I haven't had much luck trying to find a working solution to avoid this or the fact the virtual bubbling slot introduces a div when it would be preferential to avoid that inside the ToolsPanel.
One problem I ran into was that the virtual bubbling slots do not appear to accept a function as a child as per the normal slot component.
Slot also accepts optional children function prop, which takes fills as a param. It allows to perform additional processing and wrap fills conditionally.
The docs do not appear to make any distinction here. @gziolo or @youknowriad can you confirm if this is an issue with the docs or whether the virtual bubbling slot component should also offer that?
While exploring this, it looked like it might be possible to leverage the fills retrieved within the inspector controls slot and do something with them from there but that felt like the wrong path to go down. As things stand now, this PR gets the block support tools panel and slots functional. We can iterate on this in follow up PRs.
Yes, I think that might be a limitation of this approach.
The best way to approach fills is to move all logic that decides whether a fill should be rendered should be inside it. This way fill will always be rendered by React, even when completely empty, and once it has content again it will show up in the same place. A good example in gutenberg/packages/interface/src/components/complementary-area/index.js Lines 140 to 155 in c4180a8 We had a similar issue with pinned plugins in the header as reported in #28546. By the way, getting the order of fills is hard as documented in #29287. Ideally, all fills should always be registered in the same order for the given slot and they should always remain in the React tree if you want to ensure the order remains the same. |
If I understand you correctly, this is what the
I believe this is the root of the problem. The Thanks for the |
I did some more digging and my initial try would be to move gutenberg/packages/block-editor/src/hooks/dimensions.js Lines 74 to 86 in c4180a8 You can try making panelId={ ! isPaddingDisabled ? props.clientId : null }or render { ! isPaddingDisabled && <PaddingEdit { ...props } /> }The same would apply to other panels. |
Creating the ToolsPanel for block supports around the slot resulted in the slot adding an extra div within the ToolsPanel breaking its layout. This div is needed for the slot's ref to be applied allowing virtual bubbling. This change effectively moves the ToolsPanel inside that. It also avoids needing to retrieve ToolsPanelContext and recreating the context provider.
This reverts commit 94f444c.
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
from
7bc3639
to
84ba3b8
Compare
Should we use a placeholder HTML element for the fill instead in SlotFill API that doesn't have a visual representation, like |
There was a problem hiding this comment.
All of my feedback on the ToolsPanel iteself has been addressed, thank you!
Should we use a placeholder HTML element for the fill instead in SlotFill API that doesn't have a visual representation, like <script /> or ? The challenge would be to introduce a formal API method that counts the real number of fills rendered in that scenario.
This definitely sounds like an interesting suggestion, I'll let @aaronrobertshaw look into it, and more in general other reviewers to also have a final look at this PR
|
All the requested changes to date have been completed. I believe this is ready for a final review. The current list of future follow-ups includes: Work is already underway for both of these. |
|
|
||
| if ( ! isShown ) { | ||
| return null; | ||
| return shouldRenderPlaceholder ? ( | ||
| <View { ...toolsPanelItemProps } ref={ forwardedRef } /> |
There was a problem hiding this comment.
I wonder if we should force a aria-hidden="true" on this element to make sure it's ignored by assistive technology — not sure if it matters since it's an empty div 🤷
There was a problem hiding this comment.
I think it makes a difference for visual items like pictures or icons.
aaronrobertshaw
deleted the
add/virtual-bubbling-for-block-support-slots
branch
Related:
Description
Updates the Dimensions InspectorControls slot group to bubble virtually.
ToolsPanelContextand passes that throughfillPropsheaderprop fromBlockSupportToolsPanelToolsPanelContextprovider.ToolsPanelcomponent to handle injecteddivby the virtual bubblingSlot.Note: The virtually bubbling slot requires a non-functional component to apply refs to. So far I haven't found a way around this
div. As a result, this PR updates theToolsPanelstyling such that it will follow the same CSS grid layout as the panel itself.It appears this switch to virtual bubbling will also resolve a recently discovered bug with support controls which are only enabled via block specific settings within theme.json. See #34766
How has this been tested?
marginsupport via the groupblock.jsonfile.theme.jsonTypes of changes
Enhancement. Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).