search

TypeCellOS / BlockNote

A React Rich Text Editor that's block-based (Notion style) and extensible. Built on top of Prosemirror and Tiptap.
https://www.blocknotejs.org/
Mozilla Public License 2.0
5.9k stars 384 forks source link

fix: node conversion #800

Closed jkcs closed 4 weeks ago

jkcs commented 1 month ago

close #742 /claim #742

vercel[bot] commented 1 month ago

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

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Jun 6, 2024 8:40pm
blocknote-website ✅ Ready (Inspect) Visit Preview Jun 6, 2024 8:40pm
vercel[bot] commented 1 month ago

@jkcs is attempting to deploy a commit to the TypeCell Team on Vercel.

A member of the Team first needs to authorize it.

jkcs commented 1 month ago

https://github.com/TypeCellOS/BlockNote/blob/c9a53bb9bd79c3b1990f74d2c87956c3cf5949b0/packages/core/src/pm-nodes/BlockContainer.ts#L221-L224

It seems like there is the same issue here?

YousefED commented 1 month ago

Thanks @jkcs. Your solution looks great, I think you've found the right place to fix it.

https://github.com/TypeCellOS/BlockNote/blob/c9a53bb9bd79c3b1990f74d2c87956c3cf5949b0/packages/core/src/pm-nodes/BlockContainer.ts#L221-L224

It seems like there is the same issue here?

Great find! I think you're right. Could you fix this as well and create unit tests for both of them? I can double the bounty for that

jkcs commented 1 month ago

@YousefED Currently there seems to be no unit test for BNUpdateBlock. How can I add it?

YousefED commented 4 weeks ago

@jkcs the updateBlock function in https://github.com/TypeCellOS/BlockNote/blob/4a197733e46cf2d53b888e0de5db5a82792ae796/packages/core/src/api/blockManipulation/blockManipulation.test.ts indirectly call BNUpdateBlock. You can add the test there!

Can you also confirm that the test wasn't working before your fix, but now is? (to make sure the tests really work)?

jkcs commented 4 weeks ago

@YousefED Thank you for providing more information. I just tried adding tests in blockManipulation.test.ts and found that the update tests for BlockContainer.ts are not effective. The output of typeof block.content === "string" consistently shows the editor.document. I believe this test does not verify this point correctly. In the view, it should be converted to <br>.

jkcs commented 4 weeks ago

Can you also confirm that the test wasn't working before your fix, but now is? (to make sure the tests really work)?

I confirmed through snapshots that it was invalid before the fix and okay after the fix.

YousefED commented 4 weeks ago

@YousefED Thank you for providing more information. I just tried adding tests in blockManipulation.test.ts and found that the update tests for BlockContainer.ts are not effective. The output of typeof block.content === "string" consistently shows the editor.document. I believe this test does not verify this point correctly. In the view, it should be converted to <br>.

I don't understand what you mean with typeof block.content === "string" consistently shows the editor.document, can you explain?

jkcs commented 4 weeks ago

@YousefED Tests have been added and are ready for review

YousefED commented 4 weeks ago

Great!

After that, @matthewlipski can you do a final review and merge if ok?

jkcs commented 4 weeks ago

done