Fix #5620 Persist format at paragraph level for new line#5822
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
zurfyx
left a comment
There was a problem hiding this comment.
Great work, that's a hard one, some feedback:
Serialization
To support import/export we will need to add this new property onto the SerializedNode (defined above the class).
I don't think we need to handle HTML.
Updating after it's changed
The ParagraphNode currently doesn't update at all. The value is only set when creating the node. We will need to update that.
I believe a reconciler function nearby the direction logic should do the track to track the first node format.
Intentionally marked
Initially I thought we may need something like intentionally marked for selection. My hunch is that with the provided feedback I think we're good here. The selection toggle will flip when this happens and none of the other logic should override it.
But for some reason it currently doesn't work. We will need to investigate that.
(videos shows me trying to remove bold at the start of line, which currently is impossible)
Screen.Recording.2024-04-04.at.6.39.23.PM.mov
Similarly, what we should do is that when you change formatting on an empty paragraph node, we update the ParagraphNode styling.
Future work: new line after
It appears that there's a bug when the selection format carries over when deleting content.
Screen.Recording.2024-04-04.at.5.41.28.PM.mov
| ? $createParagraphNode().append(textNode) | ||
| : textNode; | ||
| textNode.setFormat(format); | ||
| const isParapgraphNode = $isParagraphNode(element); |
There was a problem hiding this comment.
| const isParapgraphNode = $isParagraphNode(element); | |
| const isParagraphNode = $isParagraphNode(element); |
| ): ParagraphNode { | ||
| const newElement = $createParagraphNode(); | ||
| if (rangeSelection != null) { | ||
| const isBackward = rangeSelection.isBackward(); |
There was a problem hiding this comment.
Might be interesting to take this up separately #5825
| restoreSelection: boolean, | ||
| ): ParagraphNode { | ||
| const newElement = $createParagraphNode(); | ||
| if (rangeSelection != null) { |
There was a problem hiding this comment.
Can it be null? The signature says otherwise
| ? this.$getParagaphNodeFromLastSelection(lastNode) | ||
| : null; | ||
| if (lastParagraphNode != null) { | ||
| newElement.setTextFormat(lastParagraphNode.getTextFormat()); |
There was a problem hiding this comment.
I don't think we want this. Let's say that I have a paragraph marked as bold but none of the elements have bold now. I don't want to carry over the bold. In this case, I think we probably still want the selection if relevant
| ? rangeSelection.anchor | ||
| : rangeSelection.focus; | ||
| const lastNode = endPoint.getNode(); | ||
| const lastParagraphNode = $isElementNode(lastNode) |
There was a problem hiding this comment.
The paragraph you're looking for is this, because we're inserting new after this paragraph
| const isParapgraphNode = $isParagraphNode(element); | ||
| textNode.setFormat( | ||
| format === 0 && isParapgraphNode && element.getChildren().length === 0 | ||
| ? element.getTextFormat() | ||
| : format, | ||
| ); |
There was a problem hiding this comment.
Should we move this logic into the selectionChange function? I was thinking that, just like when you put your caret inside a bold text the selection is bold, the same should be for an empty paragraph.
In this case, this logic should probably be the DFS backward we discussed. We might need to adjust the current heuristic. Or just handle the empty paragraph node case for now.
zurfyx
left a comment
There was a problem hiding this comment.
LGTM, thanks! Can you add a couple more tests please?
- Toggle format with CMD + B at the start of a paragraph of different format, toggled format persists
- Removing all nodes of a paragraph with format and typing (I know that behavior doesn't resemble GDocs/Word but let's at least make sure it doesn't regress).
Can we follow up on the table formatting (in a separate PR)? Particularly persisting the format on creation and adding/removing rows and columns.
|
Does this fix also applies for styling? Like text color, font family, font size, etc? |
Before
Screen.Recording.2024-04-08.at.9.32.51.AM.mov
After:
Screen.Recording.2024-04-08.at.9.34.16.AM.mov