Fix Clear Formatting (Fixes #4188) #4204
Conversation
ivailop7
requested review from
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
|
@fantactuka could you help me out how to replace a HeadingNode with a TextNode. I tried node.replace(new TextNode('abc'), false);but I get an error that I can only add an Element or a Decorator node to the Root node. What would be the right syntax here? |
|
@ivailop7 you need to wrap the TextNode in a ParagraphNode (or some other element). They can’t exist as direct children of the root. |
Understood. Thank you @acywatson . |
|
Updated. The CI failing post the Excalidraw update I believe is because of the parallelization of the tests, thus the sporadic success/failure. I believe it has to be moved from package.json inside the playground to the root one, but needs proper investigation, this is just a theory. Might dig into that one soon. |
| if (idx === 0 && anchor.offset !== 0) { | ||
| node = node.splitText(anchor.offset)[1]; | ||
| } | ||
| if (idx === nodes.length - 1) { | ||
| node = node.splitText(focus.offset)[0]; | ||
| } |
There was a problem hiding this comment.
Should we check whether format is already 0 and style is ''? This will prevent unnecesarily splitting a node
There was a problem hiding this comment.
Very good point, will amend!
| } else if ($isHeadingNode(node) || $isQuoteNode(node)) { | ||
| node.replace($createParagraphNode(), true); |
There was a problem hiding this comment.
Is this what's expected out of clear formatting? My hunch is that clear formatting should only clear the above (style + format) but the nodes should be left intact. What makes you think this is the correct behavior? I'm looking at Quip for example
There was a problem hiding this comment.
This is the behaviour in Word.
package.json
Outdated
| @@ -103,6 +103,7 @@ | |||
| "@babel/preset-flow": "^7.14.5", | |||
| "@babel/preset-react": "^7.14.5", | |||
| "@babel/preset-typescript": "^7.16.7", | |||
| "@excalidraw/excalidraw": "^0.14.2", | |||
There was a problem hiding this comment.
Let's drop this and package-lock.json, I'm not sure why GH cache is sometimes bad but we can retry when this happens. Definitely something worth investigating but the package should live inside the playground
There was a problem hiding this comment.
Yep, removing this one.
ivailop7
force-pushed
the
clear_formatting_fix
branch
from
ffb7f4e to
127d7c0
Compare
|
@zurfyx are we good with this one? |
Can you check on the test failures please? |
|
Ops, my fault. What's the right order of npm commands to repro these tests locally? When I directly run them, all tests fail, while Unit and Integration pass. |
You probably want to run |
ivailop7
force-pushed
the
clear_formatting_fix
branch
from
2cdaa35 to
efd0209
Compare
|
@zurfyx All set here. |
|
@acywatson @zurfyx I think, this one is ready to merge. |
|
Thank you Ivaylo, sorry for the delay, I completely missed the updated version |
Clear formatting respects the selection, also converts Headings and Quotes down to Paragraphs, but doesn't convert Code Blocks or any list types, matching the behaviour of other popular text editors.
Before:
clear_formatting_before.mp4
After:
clear_formatting_after.mp4