Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SlotFill: remove registration API from useSlot result #67070

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Nov 18, 2024

Follow-up to #67000 where @youknowriad pointed out in review that the API methods (registerSlot etc) returned from __experimentalUseSlot are not used anywhere. This PR removes the API methods and leaves only the slot object.

I also update the Fill component to not use useSlot at all: the register/unregisterFill can be accessed through context and to find out if there is a registered slot and whether it has a DOM element, we can look at registry.slots directly.

I was also searching for usages of __experimentalUseSlot. And it seems to be not really used by plugins. Plugins like WooCommerce or Kubio want to know whether a particular slot has registered fills. If there are no fills, they usually don't render the slot at all. They use __experimentalUseSlotFills( name ).fills.length > 0 to determine that. The __experimentalUseSlot function is called only for backward compatibility, because before #44642 there was only __experimentalUseSlot and it returned also the fills.

The only place where __experimentalUseSlot is really used is the Popover component, to determine whether a slot with given name is registered.

If we wanted to stabilize the useSlot API and remove the __experimental prefix, we need to solve these two use cases:

  1. Find out whether a slot (by name) has any registered fills. This is widely used to render slots conditionally, only if there are fills registered. Because even an empty slot renders an element and is visible in the UI, and that's often not wanted.
  2. Find out whether a slot (by name) is registered at all.

@jsnajdr jsnajdr added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Nov 18, 2024
@jsnajdr jsnajdr self-assigned this Nov 18, 2024
Copy link

github-actions bot commented Nov 18, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

...slot,
...api,
};
return { ...slot };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning { ...slot } instead of slot has two purposes:

  1. The hook always returns an object, even if the slot is undefined. Important for compatibility.
  2. The hook has a nice return type: an object with optional ref and fillProps fields.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Thanks for checking the usage in the directory.

@jsnajdr jsnajdr merged commit 9820a8c into trunk Nov 18, 2024
64 checks passed
@jsnajdr jsnajdr deleted the remove/use-slot-api branch November 18, 2024 10:58
@github-actions github-actions bot added this to the Gutenberg 19.8 milestone Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants