feat: Better parsing of external HTML attributes & inline styles #1605
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-docx-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
There was a problem hiding this comment.
👍 this looks exactly as I'd have expected it to.
Great work @matthewlipski
nperez0111
force-pushed
the
lossy-html-fixes
branch
from
e613254 to
1b23c70
Compare
packages/core/src/extensions/BackgroundColor/BackgroundColorMark.ts
Outdated
Show resolved
Hide resolved
| if (element.hasAttribute("data-text-color")) { | ||
| return { stringValue: element.getAttribute("data-text-color") }; | ||
| if (element.hasAttribute("data-text-color") || element.style.color) { | ||
| return {}; |
There was a problem hiding this comment.
Yeah we don't need to parse the color because it's already handled in the parseHTML function of the stringValue attribute.
There was a problem hiding this comment.
can't we omit line 25 / 26 then entirely? it's still a bit confusing right? (maybe tiptap issue)
There was a problem hiding this comment.
No, because return {} tells TipTap that the element should get parsed as a textColor mark, and to just use the default attribute values for that mark (here it is equivalent to return { textColor: defaultProps.textColor.default }). The parseHTML function of the stringValue attribute then does its thing, parsing the actual value of stringValue and replacing the default one. Returning false instead means the mark never gets parsed at all.
This is a kind of weird TipTap thing especially with return {}, but it does mostly make sense.
We could choose to parse the actual value of stringValue within the parseHTML function of TextColorMark, but this would result in duplicated code between TextColorMark and TextColorExtension.
packages/server-util/src/context/__snapshots__/ServerBlockNoteEditor.test.ts.snap
Outdated
Show resolved
Hide resolved
matthewlipski
changed the title
matthewlipski
changed the title
There was a problem hiding this comment.
Looks good!
Just realized one caveat; for yjs stored documents the block color data will probably be lost when upgrading (or the document might break entirely, but I doubt that).
Perhaps you can see with Nick if there's a fix for that possible (or a migration path for people upgrading)
...ages/xl-multi-column/src/test/conversions/__snapshots__/multi-column/undefined/external.html
Outdated
Show resolved
Hide resolved
packages/core/src/extensions/BackgroundColor/BackgroundColorExtension.ts
Show resolved
Hide resolved
| if (element.hasAttribute("data-text-color")) { | ||
| return { stringValue: element.getAttribute("data-text-color") }; | ||
| if (element.hasAttribute("data-text-color") || element.style.color) { | ||
| return {}; |
There was a problem hiding this comment.
can't we omit line 25 / 26 then entirely? it's still a bit confusing right? (maybe tiptap issue)
tests/src/unit/core/formatConversion/export/__snapshots__/html/hardbreak/multiple.html
Outdated
Show resolved
Hide resolved
* Added lossy HTML export/parse equality tests * Fixed list item props in external HTML * Fixed tables in `partialBlockToBlockForTesting` * Added advanced table test * Added comment * feat: Inline style props in external HTML (#1636) * Made default props on default blocks get rendered to inline styles for lossy HTML * Updated unit test snapshots * Implemented PR feedback * Small fix
This PR adds tests for parsing various HTML attributes & inline styles, and provides fixes so that these tests pass. Namely, these tests include:
startprops fromstartattribute.widthprops fromwidthattribute.bold,italic,underline, andstrikemarks from various tags & inline styles. This includes e.g.btags andfont-weightinline styles forboldmarks as well asitags andfont-styleinline styles foritalicmarks.textColorandbackgroundColormarks from inline styles.textColorandbackgroundColorprops from inline styles.textAlignmentprops from inline styles.To ensure all tests pass, several changes have also been made to the code:
textColor,backgroundColor, andtextAlignmentprops for inline styles.textColorandbackgroundColorprops have been changed, so that they're now applied as attributes toblockContentnodes instead ofblockContainernodes. This is because we would only attempt to parse these props when ablockContainernode is found, i.e. when adivwith[data-node-type="blockContainer"]is found. So we would never even attempt to parse these props from external HTML. The CSS has been updated also to account for this change. Also, all other block props are already stored on theblockContentnode, so this makes thetextColorandbackgroundColorprops also consistent with that.Additionally, code for color props & marks has been cleaned up slightly.
The large negative diff in this PR is also because a bunch of redundant React unit test snapshots have been removed.