BlockControls, InspectorControls: remove useSlot, unify behavior on bad group #50198
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
jsnajdr
added
[Type] Code Quality
|
Size Change: +13 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
youknowriad
approved these changes
May 1, 2023
jsnajdr
force-pushed
the
update/block-inspector-controls-bad-group
branch
from
ffa7e68 to
ae00c17
Compare
|
Thank you @jsnajdr for cleaning up Keeping two versions of a component around without a clear distinction is not great, IMO. It would be interesting to understand if we could keep only one, or at least merge the two implementations into one component (and switch behaviour via a prop or something similar).
|
This is exactly how The original goal was to keep only Both implementations have their pros and cons, and the crucial difference seems to be in which React context tree does the If we want to keep only one implementation, then, because of the context, it should be
I believe types should be mostly the same for both implementations. If I haven't missed anything, a |
|
Thank you for the context, @jsnajdr! In your opinion, does it make sense to assume that all instances of I'd love to make progress on the cleaning up and consolidating further
What do you think? |
|
At some point, I tried migrating all the usages to "bubblesVirtually" but I didn't manage to merge the PR, it was too big and I end up switching to different work. I still think there's value in trying to do that. I'm not 100% confident that we will be able to achieve it easily but the only way to find out whether it works for all of our instances is to actually try it I think. |
We can maybe try again, but with smaller PRs (one component at a time) which hopefully makes it much more manageable ? |
I don't think there is any general principle that says that the In a Gutenberg block, when it renders a toolbar item or sidebar item, these UI's live completely outside the "blocks view" or how it's called. They are rendered using a
You see, ideally the |
|
Thank you again for the extra context (no pun intended) ! Given the above, what next steps do you propose in order to get towards a simplification of |
|
Migrate everything to Originally I thought that the implementation without portals, the one whose |
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.
First, removes call of
useSlotfromInspectorControlsSlot. It performs no function, and should have been removed in #44642, after the introduction of theuseSlotFillhook. There was a check if the return value ofuseSlotis falsy, but it is always truthy, even if the requested slot is not found. Because it returns a combination of the slot data and the update/unregister API.Second, I'm unifying code that handles the situation where the passed
groupprop is invalid: the group is not known and has no slot associated. We don't want to crash, therefore careful use of?.. And we want to report a warning to console. I'm also updating the names of affected components toBlockControlsandInspectorControls, using plural, because that's what the name of all the components is.