WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

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

Open Mamaduka opened 9 months ago

Mamaduka commented 9 months ago

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

youknowriad commented 9 months ago

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 commented 9 months ago

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: #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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.