Skip to content

fix(table): table cell merging#1446

Merged
nperez0111 merged 2 commits into
tablesfrom
table-merge
Feb 26, 2025
Merged

fix(table): table cell merging#1446
nperez0111 merged 2 commits into
tablesfrom
table-merge

Conversation

@nperez0111
Copy link
Copy Markdown
Contributor

How crazy would it be to just allow table cells to have multiple table paragraphs to allow them to merge cleanly, then just store that back in the json cleanly?

I made this as a separate PR since the other one is getting rather large & this is more experimental

@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Feb 26, 2025 2:37pm
blocknote-website ✅ Ready (Inspect) Visit Preview Feb 26, 2025 2:37pm

@YousefED
Copy link
Copy Markdown
Collaborator

interesting! very minimal solution so could be nice. Downside is I suppose that it makes the reasoning between blocknote schema vs table schema more complex.

Where is the merge logic located in PM makes it work like this? i.e.; where are we "breaking prosemirror" by only allowing a single tableparagraph? I suppose here? https://github.com/ProseMirror/prosemirror-tables/blob/b0130d3bea42c368d38c47dcd5ff09f40dfdd33e/src/commands.ts#L435

@nperez0111
Copy link
Copy Markdown
Contributor Author

Yep so the content.append is able to add together the two tableParagraphs (which become an array of tableParagraphs at that point) which is not valid for a tablecell. So Prosemirror does it's whole "find the right place to fit the content" algo and messes up the table.

Eh, the only thing that needs to be aware of this is nodeToBlock, which should just always try to conform it into the right shape. We already make some lies about the data model, like how table headers are technically possible anywhere, but we lossily convert that to only be allowed for the headerRow & headerCol.

The alternative to this would be to do the merging logic in blocknote JSON, but that would be quite a lot of work

Copy link
Copy Markdown
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no objections except the comment added. Nice solution!

cellNode.firstChild!,
inlineContentSchema,
styleSchema
content: cellNode.content.content.flatMap((child) =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure this doesn't load any information when we load the blocknote json in the editor again? i.e.: should there be a join("\n") for example, or similar?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look at this again, but, yea there definitely is some sort of coalescing that can happen here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants