ToolsPanel: Allow SlotFill injection of panel items #34632
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronrobertshaw
added
[Feature] UI Components
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
7 tasks
|
Size Change: +284 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
7 tasks
|
I see changes in the public API but it seems to be fine gutenberg/packages/components/src/index.js Lines 141 to 144 in e3591aa |
ciampo
added
[Package] Components
ciampo
reviewed
Sep 8, 2021
There was a problem hiding this comment.
Thank you for moving changes to the ToolsPanel to a separate PR, it made this review much easier!
Code changes in this PR LGTM :) Thank you also for adding more props to the docs, and for updating unit tests!
I was playing around with the component in Storybook, and probably came across a bug: deselecting a component doesn't seem to reset its value after clicking on the "Reset All" button:
toolspanel-reset.mp4
Also, do you think it'd be valuable to add a Storybook example showing how to use the panelId props, especially in the context of injection of panel items via a SlotFill?
Although I'm ok with not adding it if you think it doesn't make much sense / bring much value.
gziolo
reviewed
Sep 8, 2021
Thanks for catching that @ciampo! When addressing the last round of feedback, I messed up the clearing of the I've added another unit test for this along with a fix. I now get the following behaviour: Screen.Recording.2021-09-09.at.2.34.42.pm.mp4
I've added another storybook example that creates its own SlotFill and uses that to illustrate injecting items into the panel. Hopefully, that makes sense and is useful. I'm not tied to it so if anyone has any suggestions on improving it or even removing it, that's fine by me. The rewording of the docs and some typo fixes have been made as well. |
gziolo
reviewed
Sep 9, 2021
gziolo
reviewed
Sep 9, 2021
|
|
||
| return ( | ||
| <SlotFillProvider> | ||
| <ToolsPanelItems> |
There was a problem hiding this comment.
This example and the documentation efforts made me think whether panelId should be part of the component. Should we create an opinionated Fill and Slot and keep them in the block editor instead?
The difference would be that ToolsPanelItem from the @wordpress/block-editor package would be a fill that handles all the logic related to panelId and renders ToolsPanelItem. There would be also a slot that would pass down panelId to fills through fillProps and would be wrapped with ToolPanel component. I don't know if I'm doing a good job explaining it.
So the goal would be to achieve the following:
const { Fill, Slot } = createSlotFill( 'ToolsPanelSlot' );
function shouldRender( panelId ) {
// checks somehow whether to render the panel based on the value passed from slot using `fillProps`
}
const ToolsPanelItemFill = ( { panelId, ...props } ) => {
return (
<Fill>
{ shouldRender( panelId ) && <ToolsPanelItem { ...props } /> }
</Fill>
);
};
const ToolsPanelSlot = ( { panelId, ...props } ) => {
return (
<ToolsPanel { ...props }>
<Slot fillProps={ panelId } />
</ToolsPanel>
);
}; It needs more code, but I hope it helps to sketch the idea.
There was a problem hiding this comment.
That's an interesting idea. I don't think I fully understand the implications yet but will explore this tomorrow.
It makes sense in terms of being able to remove the need for panelId on the ToolsPanel itself. At this stage though, I don't have much of an idea how the individual ToolsPanelItems would be working.
There was a problem hiding this comment.
You probably can play with the Storybook story first and see if you can make it work this way. As noted in another place, given that both updated components are marked as experimental don't treat my comment as a blocker.
ciampo
approved these changes
Sep 9, 2021
There was a problem hiding this comment.
Thank you @aaronrobertshaw for addressing all the feedback!
When addressing the last round of feedback, I messed up the clearing of the isResetting ref which ultimately caused the panel items to think the panel was still undertaking a "reset all" and therefore skip calling the onDeselect callbacks.
This explanation makes total sense and explains the bug — I can confirm that the component now behaves as expected!
I've added another storybook example that creates its own SlotFill and uses that to illustrate injecting items into the panel.
I agree with @gziolo's point about potentially extracting panelId (and slot fill logic) to another part of the repo, but that's definitely not a blocker and should be investigated/tackled separately.
Also, special thanks for improving unit tests with every PR!
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
Description
This updates the
ToolsPanelcomponent to support injection of panel items via a SlotFill. The changes to theToolsPanelhere have been separated from #34157.Changes include:
ToolsPanelContextso it can be passed via slots'fillPropslabelandheaderprops for theToolsPanelresetAllcallback allowing injected panel item controls to reset whatever they needHow has this been tested?
npm run test-unit packages/components/src/tools-panel/testnpm run storybook:devScreenshots
Types of changes
Enhancement. Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).