Blocks API: Adding a block uid to identify the block#361
Blocks API: Adding a block uid to identify the block#361youknowriad wants to merge 1 commit intomasterfrom
Conversation
a096578 to
9f0e681
Compare
| '<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->'; | ||
|
|
||
| expect( parse( postContent ) ).to.eql( [ { | ||
| uid: '1', |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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 } );
// ...There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You're right, but isn't that what you want? For the incoming changes to be merged with existing attributes?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I understand now. And you're right, that would be undesirable, so merge is a bad idea. 👍
| attributes: { | ||
| ...blocks[ index ].attributes, | ||
| ...changes | ||
| } |
There was a problem hiding this comment.
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'
]
}|
Maybe this is a stupid question, but why do we need to uniquely identify a block? |
|
@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. |
|
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. |
|
Closing as superseded by #368. |
In this PR, I'm adding a
uidto identify blocks. It's used right now as akeyfor the rendered block. But it's important to have for block statesselected/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
lodashsince I'm usinguniqueIdto generate the id.