Skip to content

Blocks API: Adding a block uid to identify the block#361

Closed
youknowriad wants to merge 1 commit intomasterfrom
add/block-uid
Closed

Blocks API: Adding a block uid to identify the block#361
youknowriad wants to merge 1 commit intomasterfrom
add/block-uid

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

In this PR, I'm adding a uid to identify blocks. It's used right now as a key for the rendered block. But it's important to have for block states selected / hover / focused. It allows identify the same block even if we move it up/down.

Also. See this PR as the PR we discuss adding lodash since I'm using uniqueId to generate the id.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 31, 2017
@youknowriad youknowriad self-assigned this Mar 31, 2017
@youknowriad youknowriad requested review from aduth and mtias March 31, 2017 10:54
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->';

expect( parse( postContent ) ).to.eql( [ {
uid: '1',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems fragile as it relies on uniqueId never having been called before in the current process. Instead it might be better to test that it's numeric (or even just assigned), which will probably require breaking apart the assertions to:

expect( parsed[ 0 ].uid ).to.be.a( 'number' );
// ... etc

const onChangeBlock = ( index ) => ( changes ) => {
const onChangeBlock = ( uid ) => ( changes ) => {
const index = findIndex( blocks, { uid } );
const newBlock = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Introducing lodash inclines to make suggestions 😁 _.merge could avoid the nested spread operators:

const onChangeBlock = ( uid ) => ( attributes ) => {
	const index = findIndex( blocks, { uid } );
	const newBlock = merge( {}, blocks[ index ], { attributes } );
	// ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding how merge works but I think it can cause issues if an attribute is an object. It won't replace it but merge it instead. Am I right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're right, but isn't that what you want? For the incoming changes to be merged with existing attributes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not really, If I call onChange( { myAttribute: { key1: '1', key3: '2' } } ) and I have { myAttribute: { key2: 2 } } in the current state. I don't want to keep the key2. I want to replace the attribute completely.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mmm, as documented, properties of setAttributes are applied as a partial patch, not a complete replacement. I like to think of it akin to React's setState. Otherwise it's very inconvenient to update a single attribute of a block in an input's event handler. I'd be okay with an additional replaceAttributes perhaps though if we need to support this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you're misunderstanding my point. I agree, setAttributes is a partial patch, meaning we can change a single attribute, but I was thinking inside this single attribute if this attribute is an object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I understand now. And you're right, that would be undesirable, so merge is a bad idea. 👍

attributes: {
...blocks[ index ].attributes,
...changes
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thinking about an idea state shape, it might be nice that when we receive the initial parsed set to transform it into a shape that more easily supports lookup and reordering, like:

{
	blocksByUid: {
		'jz1ajlkq': {
			blockType: 'core/text',
			attributes: {}
		},
		'kzjqoosz': {
			blockType: 'core/image',
			attributes: {}
		}
	},
	blockOrder: [
		'jz1ajlkq',
		'kzjqoosz'
	]
}

@ellatrix
Copy link
Copy Markdown
Member

ellatrix commented Apr 4, 2017

Maybe this is a stupid question, but why do we need to uniquely identify a block?

@aduth
Copy link
Copy Markdown
Member

aduth commented Apr 4, 2017

@iseulde It's a good point to raise. It seemed reasonable to want to track a block regardless of its position, but perhaps it's enough just to work with it's current index in the array of blocks?

Maybe @youknowriad had other thoughts in mind.

See also #368 which implements a UID as well.

@BE-Webdesign
Copy link
Copy Markdown
Contributor

BE-Webdesign commented Apr 4, 2017

Having UIDs would be good to have, especially in the future, for potential collaborative tools. It would be hard to make changes to blocks etc in a collaborative space purely going off of current index. I don't think it is necessary at the moment to have them, but having UIDs for things is generally a good idea, so it would probably be good to put them in now.

@aduth
Copy link
Copy Markdown
Member

aduth commented Apr 4, 2017

Closing as superseded by #368.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants