Fix duplicate block ids when duplicating stream blocks in page editor #9108
Conversation
Manage this branch in SquashTest this branch here: https://jams2fix7712-fix-duplicated-st-f8yam.squash.io |
jams2
force-pushed
the
fix/7712-fix-duplicated-streamblock-uuids
branch
from
9f792d9
to
a5ab703
Compare
| rows: this.rows.map((row) => ({ | ||
| values: row.blocks.map((block) => block.getValue()), | ||
| })), | ||
| }; | ||
| return value; | ||
| } | ||
|
|
||
| getColumnState() { |
There was a problem hiding this comment.
Nice improvement here!getColumnsState (with an s) might be ever-so-slightly clearer though, since there are usually multiple columns
There was a problem hiding this comment.
Also, do we need a getDuplicatedColumnsState(), or do columns not have ids?
There was a problem hiding this comment.
If I'm not mistaken, we don't need to handle columns any differently. TypedTableBlock.columns is a list of objects that contain metadata for the column, including:
- the block definition of the "type" that can be inserted into rows in this column; and
- an integer ID, the index of the column, assigned when the column is created.
The IDs on the row items are generated in a similar fashion. These column and row IDs don't appear to have a unique constraint as they aren't used (for example) as part of a contentpath for comments. The reason that we need special handling for rows is that the block type for a given column may be a complex type (e.g. a StreamBlock), which we would need to recurse into instances of, to make sure StreamChild, ListChild, etc. instances don't have duplicate IDs when duplicating a block.
However, this code is new to me so I'm happy to be corrected.
There was a problem hiding this comment.
Regarding the name, I've gone with getColumnStates - that feels more natural to me.
There was a problem hiding this comment.
Thanks for clarifying there @jams2. Happy with all of that :)
| for (const name in this.childBlocks) { | ||
| const block = this.childBlocks[name]; | ||
| state[name] = | ||
| block.getDuplicatedState === undefined |
There was a problem hiding this comment.
Looks like we are missing a test addition for this component?
There was a problem hiding this comment.
This was sort of covered in StreamBlock.test.js but I've split them out to handle the block types more specifically in their corresponding modules.
There was a problem hiding this comment.
@jams2 Had a good comb through, and what is here looks really great (although, I must admin, some of this JS goes over my head!).
One additional thing that occurred to me, though: Does the ListBlock component need updating too?
|
Thanks @ababic ! Going to mark this as a draft while I investigate the |
jams2
force-pushed
the
fix/7712-fix-duplicated-streamblock-uuids
branch
from
9035eeb
to
7df7f7a
Compare
Fixes duplication of block ids when duplicating stream field blocks
jams2
force-pushed
the
fix/7712-fix-duplicated-streamblock-uuids
branch
from
7df7f7a
to
f9a3791
Compare
jams2
force-pushed
the
fix/7712-fix-duplicated-streamblock-uuids
branch
from
6edc56d
to
5cb6297
Compare
fixes #7712 (duplication of block ids when duplicating stream field blocks in page editor), supersedes #7713.
This implementation (based on @gasman and @jacobtoppm 's comments on #7713) adds
getDuplicatedStatemethods toBaseSequenceChild,BaseSequenceBlock(inherited byListBlockandStreamChild),StreamChild, andStructBlock.getDuplicatedStategenerates new block ids for stream/list children (at all levels of nesting) when duplicating blocks. It is called in theduplicate/duplicateBlockmethods of compound block types.Tested manually that block duplication still works as expected in the page editor in Firefox and Chrome.