Update the block styles registration to avoid timing issues #11532

youknowriad · 2018-11-06T09:41:52Z

At the moment, registering blockStyle variations is very fragile because you have to register it before the block registers itself and at the same time the block registers itself before the dom is ready which makes it very hard to do properly.

In this PR, I'm updating the block styles registration:

Moving away from filters
Using a separate reducer to keep track of block styles
Instead of relying on getBlockType to fetch the styles, rely on a selector (reactive)

This way we can register/unregister block styles at any moment.

I'd also argue that we should stop using getBlockType and getBlockTypes entirely and try to refactor our code to use the selectors instead. So any change that would happen to the block settings after the initial registration would be taken into consideration in the UI.

Testing instructions

Open the console and register a style variation: wp.blocks.registerBlockStyle( 'core/image', { name: 'fancy' } )
Insert an image, you should see the block style variation in the block switcher menu.

jorgefilipecosta

This PR seems to work correctly. I left some improvements I think should be made before the merge.

I understand the reason for this change and it works well 👍 But I would prefer if blockStyles appeared under blockTypes.[blockName].blockStyles instead of blockStyles[blockName] in the state structure.
Would force the addBlockStyles call to be made after the block being registered to be an option?
If not would the concept of temporaryBlockType be an option? When calling addBlockStyles in a non-existing block it would add the style part in temporaryBlockType a sibling blockTypes of when registering the block if the block contains properties in temporaryBlockType these properties would be moved. This would be a totally generic solution, and in the future, if we have other need to dynamically manage or add block properties this system would work.

This last part is just a personal option regarding the state structure, it is a non-blocker at all.

jorgefilipecosta · 2018-11-08T17:59:22Z

packages/blocks/src/store/reducer.js

+ */
+export function blockStyles( state = {}, action ) {
+	switch ( action.type ) {
+		case 'ADD_BLOCK_TYPES':


I think we also should handle the REMOVE_BLOCK_TYPES action.

I didn't explicitly because the registration of styles can happen even before the block exists or after, that's one of the main ideas of this PR. You can register your styles and your block whenever you want, the styles will always be there.

For example, a plugin author could unregister a block type, register it back slightly modified and the styles will still be there.

Interesting way of thinking. The issue I see here is that you could add styles with block X and then remove it but its styles will be still there.

Is this really an issue? I mean aside some state being there and not really useful.

It would be an issue if you reuse the same block name to register your own version of the block, it would have also styles from the old block. Not a big deal in my opinion, but it raises a flag if you would scale the pattern for other parts of the block settings. I'm thinking how to turn block.registerBlockType hook into a self-dispatching action which does the same what you did here :)

jorgefilipecosta · 2018-11-08T18:11:15Z

packages/blocks/src/store/reducer.js

 	return state;
 }

+/**


I think we also need to change blockTypes to remove styles from the blockTypes, right now they are duplicated in blockTypes and blockStyes.

I thought about it. I actually even had it implemented, but decided to revert. The idea is that, this could serve as a way to differentiate which styles are custom and which are defined in the block settings. But I don't have strong opinions though, I'm fine omitting them.

youknowriad · 2018-11-08T22:08:49Z

Would force the addBlockStyles call to be made after the block being registered to be an option?

That's not an option, the idea of this PR is to allow plugins to register styles whenever they want regardless of existance of blocks or not. Because the loading order of JS scripts is very fragile if we can avoid it being an issue, then it's better for everyone.

youknowriad · 2018-11-08T22:10:23Z

But I would prefer if blockStyles appeared under blockTypes.[blockName].blockStyles instead of blockStyles[blockName]

I don't have strong opinion on the state shape, it's just implementation detail for me as the most important part is the selectors. Changing as you suggest means I have to also refactor the block types to be something like blockTypes.[blockName].types, I just think it's not necessary now.

jorgefilipecosta

This seems to work correctly on my tests. Thank @youknowriad for clarifying my questions. I understood the rationale behind the decisions taken here and this looks ready to go 👍

jorgefilipecosta · 2018-11-09T09:20:52Z

packages/editor/src/components/block-styles/index.js

 	onHoverClassName = noop,
 } ) {
-	if ( ! styles ) {
+	if ( ! styles || styles.length === 0 ) {


Even if the block has no styles at all, it contains an empty array of styles. Should we continue to check for the length?

yes, the idea is that we don't show this panel at all if there's no styles.

swissspidy · 2018-11-28T11:19:40Z

Seems like one still has to use wp.domReady in order to (un)register styles...

youknowriad · 2018-11-28T11:29:47Z

@swissspidy Yes probably because if you unregister a style and then a code runs later and register it back, it will be there.

erikjoling · 2018-11-28T13:10:27Z

Seems like one still has to use wp.domReady in order to (un)register styles...

Registering custom styles works fine for me. Unregistering core/block styles is another story.

sirreal · 2020-07-29T14:43:27Z

packages/blocks/src/store/reducer.js

 				...state,
-				...keyBy( action.blockTypes, 'name' ),
+				...keyBy(
+					map( action.blockTypes, ( blockType ) => omit( blockType, 'styles ' ) ),


@youknowriad This looks like a typo, should 'styles ' be 'styles'?

It's present in current releases:

gutenberg/packages/blocks/src/store/reducer.js

Line 58 in 71bf1b5

omit( blockType, 'styles ' )

It looks like this was intended to exclude the styles property in blockTypes state (it's moved to blockStyles), but it's present:

wp.data.select('core/blocks').getBlockType("core/image").styles.length // 2

definitely looks like a typo yes, do you know what concrete impact this has?

I noticed it when inspecting some code, I don't have much idea of the impact it has.

It does appear to duplicate the state stored in blockStyles:

wp.data.select('core/blocks').getBlockType("core/image").styles // [{"name":"default","label":"Default","isDefault":true},{"name":"rounded","label":"Rounded"}] // Gives same result wp.data.select('core/blocks').getBlockStyles("core/image") // [{"name":"default","label":"Default","isDefault":true},{"name":"rounded","label":"Rounded"}] // But not referential equality wp.data.select('core/blocks').getBlockType("core/image").styles === wp.data.select('core/blocks').getBlockStyles("core/image") // false

youknowriad added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Code Quality Issues or PRs that relate to code quality labels Nov 6, 2018

youknowriad self-assigned this Nov 6, 2018

youknowriad requested a review from a team November 6, 2018 09:41

youknowriad added this to the 4.3 milestone Nov 7, 2018

jorgefilipecosta reviewed Nov 8, 2018

View reviewed changes

youknowriad added 4 commits November 9, 2018 09:28

Update the block styles registration to avoid timing issues

5e4cee8

Update changelogs

29503ed

Update data docs

eda408a

Fix e2e tests

c89b880

youknowriad force-pushed the update/block-styles branch from 2c98e23 to c89b880 Compare November 9, 2018 08:29

Omit styles from the block settings object

484fcfb

jorgefilipecosta approved these changes Nov 9, 2018

View reviewed changes

youknowriad merged commit 448d9f4 into master Nov 9, 2018

youknowriad deleted the update/block-styles branch November 9, 2018 09:54

youknowriad mentioned this pull request Nov 9, 2018

Where to add JavaScript code for registerBlockStyle() #11392

Closed

mario-neuhold mentioned this pull request Nov 21, 2018

Is it possible to disable default block styles? #11338

Closed

sirreal reviewed Jul 29, 2020

View reviewed changes

Update the block styles registration to avoid timing issues #11532

Update the block styles registration to avoid timing issues #11532

Uh oh!

Conversation

youknowriad commented Nov 6, 2018

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 8, 2018

Uh oh!

youknowriad commented Nov 8, 2018

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swissspidy commented Nov 28, 2018

Uh oh!

youknowriad commented Nov 28, 2018

Uh oh!

erikjoling commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gziolo Nov 9, 2018 •

edited

Loading