Size Change: +156 B (0%)

Total Size: 1.04 MB

_{compressed-size-action}

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 group was marked as experimental based on the feedback received, so it's now __experimentalGroup.

I like this approach personally. But you worked on both, you'd have a better insight here, what are your current thoughts?

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 updateBlockAttributes instead of the setAttributes passed through props. That ultimately led to tightening up the ToolsPanel which is another win.

I'll close the original PR in favour of this one.

@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.

Sorry for the late review, but I've only just realised this PR was in the works!

It's very much appreciated. My apologies for missing the components label on this PR and specific components squad members from the reviewers.

Also, it'd be great if in the future we could avoid mixing block editor and @wordpress/components changes within the same PR

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.

The updates to the ToolsPanel required to facilitate support for injecting panel items via SlotFills have been split into a separate PR #34632.

This PR now targets #34632 as its base.

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.

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 @wordpress/components that the community could learn about. Fortuntately there are ways to get better overview about all the changes proposed in PRs:

• everyone willing to review changed to packages/components folder can add their GitHub handles to the CODOWNERS file to get notification when PR gets opened that modified files in the folder:
  

  gutenberg/.github/CODEOWNERS

  Line 79 in e3591aa

  /packages/components @ajitbohra @jaymanpandya @chrisvanpatten

• we discussed it in the past, but we could build GitHub action that automatically applies labels based on the type of modified files
• more time-consuming approach is to go through the list of commits and opened PRs and scan for related changed on a regular schedule (week or month)
• even when something get merged, it doesn't mean it can't be reverted or updated

Also, it'd be great if in the future we could avoid mixing block editor and @wordpress/components changes within the same PR

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:

1. ToolsPanel: Allow SlotFill injection of panel items #34632 waits for review from @ciampo, once it's approved we can merge and rebase this PR with trunk
2. We seem to agree that this PR can land as is after point 1 happens.
3. I discussed with @aaronrobertshaw that he will open a follow-up PR after point 2 happens. The scope will be limited to the changes to the SlotFill implementation as discussed in InspectorControls: Wrap block support slots in ToolsPanel #34157 (comment). I'm happy to help with finding a working solution. @diegohaz and everyone else who find it intriguing you are all welcome to join efforts 😄

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.

There are a few reasons behind my previous suggestion about keeping changes to the @wordpress/components package separate from the rest of the changes.

Firstly, splitting PRs into smaller chunks makes them easier to review and, in particular, isolating changes to the @wordpress/components package makes it easier to search/track/understand them at a later time (for example, for tracking down the root cause of a bug and issuing a fix).

But most importantly, the @wordpress/components package aims at providing a base for building UIs in Gutenberg (and beyond). Changes made to this package need to be considered more carefully compared to standard "product" work, since they will have repercussions for all of the users of this package (and not only the Gutenberg repo). This is even more important when considering how much we value not introducing breaking changes.

The @wordpress/components package still contains legacy components that are not always consistent with each other and that feel like "copy-pasted app components", lacking that level of polish and thoughtfulness that should be part of a good components library. Our recent efforts, like updating the contributing guidelines and investigating APIs consistency, aimed at improving the situation in these areas.

With this perspective in mind, I don't see this being a limitation — rather an improvement in how we collaborate on the @wordpress/components package, which should help to increase its overall quality and consistency.

This should hopefully also give more context on the other point about involving the components squad:

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 @wordpress/components that the community could learn about.

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.

Firstly, splitting PRs into smaller chunks makes them easier to review and, in particular, isolating changes to the @wordpress/components package makes it easier to search/track/understand them at a later time (for example, for tracking down the root cause of a bug and issuing a fix).

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 @wordpress/components as discussed above.

The @wordpress/components package still contains legacy components that are not always consistent with each other and that feel like "copy-pasted app components", lacking that level of polish and thoughtfulness that should be part of a good components library.

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 😄

Just gave this is another smoke test, and it's working nicely for me with the useEffect added back in for clearing the isResetting value. Looks like that now ensures that clearing the value happens after the render is completed, instead of part way through 👍