fix: Deleting last block in column #2110
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@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: |
packages/core/src/api/blockManipulation/commands/replaceBlocks/replaceBlocks.ts
Show resolved
Hide resolved
| /** | ||
| * Moves the first block in a column to the previous/next column and handles | ||
| * all necessary collapsing of `column`/`columnList` nodes. Only moves the | ||
| * block to the start of the next column if it's in the first column. | ||
| * Otherwise, moves the block to the end of the previous column. | ||
| * @param tr The transaction to apply changes to. | ||
| * @param blockBeforePos The position just before the first block in the column. | ||
| * @returns The position just before the block, after it's moved. | ||
| */ | ||
| export function moveFirstBlockInColumn( | ||
| tr: Transaction, | ||
| blockBeforePos: ResolvedPos, | ||
| ): ResolvedPos { |
There was a problem hiding this comment.
I read this like 3 times, and couldn't understand what it was doing
There was a problem hiding this comment.
It's hard for me to evaluate this logic, I don't completely understand what this logic is trying to achieve. What I was sort of expecting from this was that given a transaction where some operation involving a column was involved, this would "fix up" the columns to make them make sense (i.e. remove columns/columnLists etc.). I'm unsure whether this is feasible, but I prefer something more generic, than trying to infer things while content is being moved around
There was a problem hiding this comment.
I also think a function that applies a transaction to check and clean up a given invalid columnList makes more sense tbh.
The reason this function is the way it is, is just to maximize how much code can be reused from the existing keyboard handler. Since the logic is fairly complex, I felt like refactoring was gonna put me down a rabbithole and I didn't wanna spend too much time on it. If you think it's worth it though, I can spend a day or 2 refactoring it into a fixColumnList function or smth along those lines.
There was a problem hiding this comment.
Maybe this is worth doing together. The way that I see this implemented would follow what you outlined in the comment here:
There are 3 different cases:
a) remove entire column list (if no columns would be remaining)
b) remove just a column (if no blocks inside a column would be remaining)
c) keep columns (if there are blocks remaining inside a column)
So, what I'd do is these 3 passes in reverse order. Something like:
- Check that a column has non-empty blocks within it
- If it does, then it is valid, & bail
- If not, then empty the column of any blocks it may have & continue
- Check if a column is empty
- If it is, remove it
- If not, bail
- Check that a columnList has only 1 column
- If it only has 1, take out it's contents, & remove the columnList completely
- If not, bail
Theoretically, at the end of all of these passes, we should be left with a valid column structure
…olumns are removed + added more tests
2 participants
Summary
Currently, when there is only one block in a column, removing it via
removeBlocks/replaceBlockswon't collapse the column as it should, leading to confusing UX when deleting the block from the side menu. This behavior is also different to removing the block by hitting backspace at its start.This PR fixes the behavior for removing the only block in a column using
removeBlocks/replaceBlocks. The logic is now shared between those functions and the keyboard handler. The only difference is that the keyboard handler moves the block to the previous/next column, whereasremoveBlocks/replaceBlocksalso removes it after.Closes #1323
Rationale
The current behavior of the remove block button in the side menu is unintuitive when used on the only block in a column, and should be improved.
Changes
moveFirstBlockInColumnfunction.removeAndInsertBlocksand relevant keyboard handler usemoveFirstBlockInColumn.TODO:
Impact
Since
removeAndInsertBlockscan now removecolumn/columnListnodes that it's actively traversing indoc.descendants, there's an edge case where I'm not sure what will happen.E.g. consider this structure:
columnListcolumnblockContainer1columnblockContainer2columnblockContainer3If we call
removeBlocksfor bothblockContainer2 & 3, I'm not sure this will work properly. Worth adding tests for this?EDIT: Added tests, works ok for both removing and replacing blocks 👍
Testing
See above.
Screenshots/Video
N/A
Checklist
Additional Notes