feat: Clean up invalid multi-column handling #2113
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
matthewlipski
changed the title
fixColumnList
packages/core/src/api/blockManipulation/commands/replaceBlocks/replaceBlocks.ts
Outdated
Show resolved
Hide resolved
packages/core/src/extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
Outdated
Show resolved
Hide resolved
| const firstColumnEmpty = isEmptyColumn(firstColumn); | ||
| const lastColumnEmpty = isEmptyColumn(lastColumn); |
There was a problem hiding this comment.
I'd like to see what happens when the first & last column are the same
There was a problem hiding this comment.
I decided to throw an error in this case. Since if the columnList only has a single non-empty column, this is both an unexpected case that should never happen (PM should automatically add an empty column to make the columnList valid), and it would not be covered by the existing cases.
| for ( | ||
| let columnIndex = columnList.childCount - 1; | ||
| columnIndex >= 0; | ||
| columnIndex-- | ||
| ) { |
There was a problem hiding this comment.
Is this not just a columnList.children.forEach?
There was a problem hiding this comment.
Eh imo it's way more intuitive in this case to use a for loop since we can go backwards. That way we don't have to worry about having to terminate early.
The bigger issue though is that forEach doesn't account for children being removed - e.g. removing child 0 should mean that child 3 now becomes child 2, but it remains child 3 in forEach, and the index ends up being wrong. So overall it's just way easier to use the reverse for loop.
packages/core/src/extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
Show resolved
Hide resolved
packages/core/src/extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
Outdated
Show resolved
Hide resolved
|
@matthewlipski, Unsure what the right thing to do here is, but when dragging a block out of a column, I'd expect the column to collapse unless it had content. Maybe the fixColumns should run as an appendTransaction in a plugin so we don't have to specifically run it everywhere that a column may be touched in some way? Screen.Recording.2025-10-20.at.17.05.06.mov |
|
Good point, though I would do that in a separate PR though since I want to get it out soon so we can close #1323, and I feel like figuring out how to nicely get the positions of changed tables after each transaction might take some time. |
matthewlipski
changed the base branch from
multi-column-block-delete-fix
to
main
matthewlipski
changed the base branch from
main
to
multi-column-block-delete-fix
2 participants
Summary
This PR replaces the
moveFirstBlockInColumnfunction withfixColumnList. While the old function reused a lot of code from the existing backspace keyboard handler for the first block in a column, it was unclear and hard to understand. The new function is much simpler, as it just takes acolumnListnode and deletes any empty columns. If all columns are empty, the whole node is deleted.Rationale
Makes the code easier to understand and maintain.
Changes
moveFirstBlockInColumn.fixColumnList.removeAndInsertBlocks.Impact
N/A
Testing
All existing tests for removing/replacing blocks/columns pass. All cases for backspace handling have been manually tested.
Screenshots/Video
N/A
Checklist
Additional Notes