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

Blocks: Introduce 'getBlockIcon' and 'getBlockTitle` selectors #58641

Closed
Mamaduka opened this issue Feb 3, 2024 · 8 comments
Closed

Blocks: Introduce 'getBlockIcon' and 'getBlockTitle` selectors #58641

Mamaduka opened this issue Feb 3, 2024 · 8 comments
Labels
[Package] Blocks /packages/blocks [Type] New API New API to be used by plugin developers or package users.

Comments

@Mamaduka
Copy link
Member

Mamaduka commented Feb 3, 2024

A proposal to introduce getBlockIcon and getBlockTitle selectors for the Blocks store.

const icon = getBlockIcon( blockName, attributes );
const title = getBlockTitle( blockName, attributes );

Why

These values are crucial to the Block's identity and accessibility. They are fetched and rendered repeatedly throughout the editors. So, they should be easily accessible via selectors instead of relying on custom hooks.

cc @WordPress/gutenberg-core

@Mamaduka Mamaduka added [Package] Blocks /packages/blocks [Type] New API New API to be used by plugin developers or package users. labels Feb 3, 2024
@youknowriad
Copy link
Contributor

We already have a BlockIcon and a BlockTitle component with some options. I believe these components address these use-cases already. So why selectors?

@Mamaduka
Copy link
Member Author

Mamaduka commented Feb 4, 2024

The BlockIcon is a tiny wrapper around the Icon component and requires passing the icon prop, which consumers should select from the store.

It's hard to use BlockTitle with compound labels like - Options for {blockTitle}.

So why selectors?

Mainly for two reasons:

  1. Provide a standardized way to select these values.
    • icon - core uses the useBlockDisplayInformation hook or duplicates its logic in the selectors. You can't specify the needed block information, so the hook constantly over-selects.
    • title - a mixed usage of the BlockTitle component, the useBlockDisplayTitle and useBlockDisplayInformation hooks. The latter does not take the __experimentalLabel method into consideration.
  2. Make it easier to fine-tune selectors for performance — an example: ListView: reduce subs per item #58542.
    • Each of these hooks creates an extra subscription to the block-editor store, which can affect typing performance.

Sorry, I have trouble writing lengthy arguments due to a dislocated shoulder, but I hope I summarize it OK :)

@noisysocks
Copy link
Member

noisysocks commented Feb 4, 2024

Why a selector instead of a hook?

If we're duplicating the logic of useBlockDisplayInformation all over the place (are we?) then my instinct would be to split it into two hooks and move them to @wordpress/blocks.

@Mamaduka
Copy link
Member Author

Mamaduka commented Feb 5, 2024

The hooks create extra store subscriptions, and as I mentioned, it can affect performance. A big part of recent perf improvements was to reduce the number of store subs. See #57935.

Also, why not the selectors? While I am happy to argue about the pros, nobody provided cons for introducing them.

@noisysocks
Copy link
Member

Also, why not the selectors? While I am happy to argue about the pros, nobody provided cons for introducing them.

Selectors are usually fairly low level, tightly coupled to the store, and focused on retrieving or aggregating the state that's in that store. It's strange to me when a selector calls into another store, which these ones would need to do, though admittedly we already have plenty of this happening in Gutenberg via createRegistrySelector.

And it's strange to me that "presentation logic" such as this wouldn't be done in something quite high level i.e. a component or hook.

But I suppose if it's quite common that we are duplicating this logic in various useSelects around the codebase then a selector makes sense.

It's hard to use BlockTitle with compound labels like - Options for {blockTitle}.

Would it work if we used the pattern common in @wordpress/components and have a useBlockTitle hook that is analogous to BlockTitle and useBlockIcon which is analogous to BlockIcon?

A proposal to introduce getBlockIcon and getBlockTitle selectors for the Blocks store.

I think the selectors would have to exist in @wordpress/block-editor because it needs access to things like getReusableBlockTitle that @wordpress/blocks ought to not care about.

@Mamaduka
Copy link
Member Author

Mamaduka commented Feb 5, 2024

We don't need data from the different stores if we pass the attributes as an argument. I believe a few selectors already use this pattern in the block store.

There's already a useBlockDisplayTitle hook used by BlockTitle. Sure, we can introduce a similar one for BlockIcon. But again, this won't solve the code duplication problem in components where selectors have to be optimized for perf - one large selector getting all the data instead of multiple.

I think the selectors would have to exist in @wordpress/block-editor because it needs access to things like getReusableBlockTitle that @wordpress/blocks ought to not care about.

Not anymore (See #58646). The blocks should use __experimentalLabel for custom label handlers, and it's part of the block definition.

@youknowriad
Copy link
Contributor

I think it's hard to say whether it should be a selector/hook/component. You mention that we need this in a few places and I think that's concrete evidence that we need something at least. Let's try to get something in and use it in all these places and see how it feels in the PR? Also we could see clearly the impact on performance.

@Mamaduka
Copy link
Member Author

I'm going to close this proposal. The current hooks are good enough, and there's no apparent gain in terms of performance. If the block title or information hook becomes a bottle next in the app, I'll revisit this.

@Mamaduka Mamaduka closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Blocks /packages/blocks [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

No branches or pull requests

3 participants