Skip to content

Commit 4ff328f

Browse files
matthewlipskimatthewlipski
committed
Implemented PR feedback
1 parent ff199d9 commit 4ff328f

File tree

4 files changed

+202
-37
lines changed

4 files changed

+202
-37
lines changed

‎packages/core/src/api/blockManipulation/commands/replaceBlocks/replaceBlocks.ts‎

Lines changed: 10 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export function removeAndInsertBlocks<
3737
),
3838
);
3939
const removedBlocks: Block<BSchema, I, S>[] = [];
40+
const columnListPositions = new Set<number>();
4041

4142
const idOfFirstBlock =
4243
typeof blocksToRemove[0] === "string"
@@ -73,34 +74,14 @@ export function removeAndInsertBlocks<
7374
const oldDocSize = tr.doc.nodeSize;
7475

7576
const $pos = tr.doc.resolve(pos - removedSize);
77+
78+
if ($pos.node().type.name === "column") {
79+
columnListPositions.add($pos.before(-1));
80+
} else if ($pos.node().type.name === "columnList") {
81+
columnListPositions.add($pos.before());
82+
}
83+
7684
if (
77-
node.type.name === "column" &&
78-
node.attrs.id !== $pos.nodeAfter?.attrs.id
79-
) {
80-
// This is a hacky work around to handle removing all columns in a
81-
// columnList. This is what happens when removing the last 2 columns:
82-
//
83-
// 1. The second-to-last `column` is removed.
84-
// 2. `fixColumnList` runs, removing the `columnList` and inserting the
85-
// contents of the last column in its place.
86-
// 3. `removedSize` increases not just by the size of the second-to-last
87-
// `column`, but also by the positions removed due to running
88-
// `fixColumnList`. Some of these positions are after the contents of the
89-
// last `column`, namely just after the `column` and `columnList`.
90-
// 3. `tr.doc.descendants` traverses to the last `column`.
91-
// 4. `removedSize` now includes positions that were removed after the
92-
// last `column`. This causes `pos - removedSize` to point to an
93-
// incorrect position, as it expects that the difference in document size
94-
// accounted for by `removedSize` comes before the block being removed.
95-
// 5. The deletion is offset by 3, because of those removed positions
96-
// included in `removedSize` that occur after the last `column`.
97-
//
98-
// Hence why we have to shift the start of the deletion range back by 3.
99-
// The offset for the end of the range is smaller as `node.nodeSize` is
100-
// the size of the second `column`. Since it's been removed, we actually
101-
// care about the size of its children - a difference of 2 positions.
102-
tr.delete(pos - removedSize + 3, pos - removedSize + node.nodeSize + 1);
103-
} else if (
10485
$pos.node().type.name === "blockGroup" &&
10586
$pos.node($pos.depth - 1).type.name !== "doc" &&
10687
$pos.node().childCount === 1
@@ -113,12 +94,6 @@ export function removeAndInsertBlocks<
11394
tr.delete(pos - removedSize, pos - removedSize + node.nodeSize);
11495
}
11596

116-
if ($pos.node().type.name === "column") {
117-
fixColumnList(tr, $pos.before(-1));
118-
} else if ($pos.node().type.name === "columnList") {
119-
fixColumnList(tr, $pos.before());
120-
}
121-
12297
const newDocSize = tr.doc.nodeSize;
12398
removedSize += oldDocSize - newDocSize;
12499

@@ -135,6 +110,8 @@ export function removeAndInsertBlocks<
135110
);
136111
}
137112

113+
columnListPositions.forEach((pos) => fixColumnList(tr, pos));
114+
138115
// Converts the nodes created from `blocksToInsert` into full `Block`s.
139116
const insertedBlocks = nodesToInsert.map((node) =>
140117
nodeToBlock(node, pmSchema),

‎packages/core/src/api/blockManipulation/commands/replaceBlocks/util/fixColumnList.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { ReplaceAroundStep } from "prosemirror-transform";
44

55
/**
66
* Checks if a `column` node is empty, i.e. if it has only a single empty
7-
* block.
7+
* paragraph.
88
* @param column The column to check.
99
* @returns Whether the column is empty.
1010
*/
@@ -26,7 +26,7 @@ export function isEmptyColumn(column: Node) {
2626
return (
2727
column.childCount === 1 &&
2828
blockContainer.childCount === 1 &&
29-
blockContent.type.spec.content === "inline*" &&
29+
blockContent.type.name === "paragraph" &&
3030
blockContent.content.content.length === 0
3131
);
3232
}
@@ -63,7 +63,7 @@ export function removeEmptyColumns(tr: Transaction, columnListPos: number) {
6363
}
6464

6565
if (isEmptyColumn(column)) {
66-
tr.delete(columnPos, columnPos + column?.nodeSize);
66+
tr.delete(columnPos, columnPos + column.nodeSize);
6767
}
6868
}
6969
}
@@ -93,11 +93,21 @@ export function fixColumnList(tr: Transaction, columnListPos: number) {
9393
}
9494

9595
if (columnList.childCount > 2) {
96-
// Do nothing if the `columnList` has at least two non-empty `column`s.
96+
// Do nothing if the `columnList` has more than two non-empty `column`s. In
97+
// the case that the `columnList` has exactly two columns, we may need to
98+
// still remove it, as it's possible that one or both columns are empty.
99+
// This is because after `removeEmptyColumns` is called, if the
100+
// `columnList` has fewer than two `column`s, ProseMirror will re-add empty
101+
// `column`s until there are two total, in order to fit the schema.
97102
return;
98103
}
99104

100105
if (columnList.childCount < 2) {
106+
// Throw an error if the `columnList` has fewer than two columns. After
107+
// `removeEmptyColumns` is called, if the `columnList` has fewer than two
108+
// `column`s, ProseMirror will re-add empty `column`s until there are two
109+
// total, in order to fit the schema. So if there are fewer than two here,
110+
// either the schema, or ProseMirror's internals, must have changed.
101111
throw new Error("Invalid columnList: contains fewer than two children.");
102112
}
103113

‎packages/xl-multi-column/src/test/commands/__snapshots__/replaceBlocks.test.ts.snap‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,169 @@
11
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
22

3+
exports[`Test replaceBlocks > Replace all blocks in column with single block 1`] = `
4+
[
5+
{
6+
"children": [
7+
{
8+
"children": [],
9+
"content": [
10+
{
11+
"styles": {},
12+
"text": "Nested Paragraph 0",
13+
"type": "text",
14+
},
15+
],
16+
"id": "nested-paragraph-0",
17+
"props": {
18+
"backgroundColor": "default",
19+
"textAlignment": "left",
20+
"textColor": "default",
21+
},
22+
"type": "paragraph",
23+
},
24+
],
25+
"content": [
26+
{
27+
"styles": {},
28+
"text": "Paragraph 0",
29+
"type": "text",
30+
},
31+
],
32+
"id": "paragraph-0",
33+
"props": {
34+
"backgroundColor": "default",
35+
"textAlignment": "left",
36+
"textColor": "default",
37+
},
38+
"type": "paragraph",
39+
},
40+
{
41+
"children": [],
42+
"content": [
43+
{
44+
"styles": {},
45+
"text": "Paragraph 1",
46+
"type": "text",
47+
},
48+
],
49+
"id": "paragraph-1",
50+
"props": {
51+
"backgroundColor": "default",
52+
"textAlignment": "left",
53+
"textColor": "default",
54+
},
55+
"type": "paragraph",
56+
},
57+
{
58+
"children": [
59+
{
60+
"children": [
61+
{
62+
"children": [],
63+
"content": [
64+
{
65+
"styles": {},
66+
"text": "New Paragraph",
67+
"type": "text",
68+
},
69+
],
70+
"id": "0",
71+
"props": {
72+
"backgroundColor": "default",
73+
"textAlignment": "left",
74+
"textColor": "default",
75+
},
76+
"type": "paragraph",
77+
},
78+
],
79+
"content": undefined,
80+
"id": "column-0",
81+
"props": {
82+
"width": 1,
83+
},
84+
"type": "column",
85+
},
86+
{
87+
"children": [
88+
{
89+
"children": [],
90+
"content": [
91+
{
92+
"styles": {},
93+
"text": "Column Paragraph 2",
94+
"type": "text",
95+
},
96+
],
97+
"id": "column-paragraph-2",
98+
"props": {
99+
"backgroundColor": "default",
100+
"textAlignment": "left",
101+
"textColor": "default",
102+
},
103+
"type": "paragraph",
104+
},
105+
{
106+
"children": [],
107+
"content": [
108+
{
109+
"styles": {},
110+
"text": "Column Paragraph 3",
111+
"type": "text",
112+
},
113+
],
114+
"id": "column-paragraph-3",
115+
"props": {
116+
"backgroundColor": "default",
117+
"textAlignment": "left",
118+
"textColor": "default",
119+
},
120+
"type": "paragraph",
121+
},
122+
],
123+
"content": undefined,
124+
"id": "column-1",
125+
"props": {
126+
"width": 1,
127+
},
128+
"type": "column",
129+
},
130+
],
131+
"content": undefined,
132+
"id": "column-list-0",
133+
"props": {},
134+
"type": "columnList",
135+
},
136+
{
137+
"children": [],
138+
"content": [
139+
{
140+
"styles": {},
141+
"text": "Paragraph 2",
142+
"type": "text",
143+
},
144+
],
145+
"id": "paragraph-2",
146+
"props": {
147+
"backgroundColor": "default",
148+
"textAlignment": "left",
149+
"textColor": "default",
150+
},
151+
"type": "paragraph",
152+
},
153+
{
154+
"children": [],
155+
"content": [],
156+
"id": "trailing-paragraph",
157+
"props": {
158+
"backgroundColor": "default",
159+
"textAlignment": "left",
160+
"textColor": "default",
161+
},
162+
"type": "paragraph",
163+
},
164+
]
165+
`;
166+
3167
exports[`Test replaceBlocks > Replace all blocks in columns with single block 1`] = `
4168
[
5169
{

‎packages/xl-multi-column/src/test/commands/replaceBlocks.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,20 @@ describe("Test replaceBlocks", () => {
3838
expect(getEditor().document).toMatchSnapshot();
3939
});
4040

41+
it("Replace all blocks in column with single block", () => {
42+
getEditor().replaceBlocks(
43+
["column-paragraph-0", "column-paragraph-1"],
44+
[
45+
{
46+
type: "paragraph",
47+
content: "New Paragraph",
48+
},
49+
],
50+
);
51+
52+
expect(getEditor().document).toMatchSnapshot();
53+
});
54+
4155
it("Replace all blocks in columns with single block", () => {
4256
getEditor().replaceBlocks(
4357
[

0 commit comments

Comments
 (0)