fix(text-block): preserve line breaks on plain text #11794
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,11 @@ export default class InsertItemCommand extends Command { | |
| const modelFragment = editor.data.toModel(viewFragment) | ||
| editor.model.insertContent(modelFragment) | ||
| } else { | ||
| const lines = item.content.split('\n') | ||
| const htmlContent = lines.map(line => `<p>${line}</p>`).join('') | ||
|
Member
There was a problem hiding this comment. I'm skeptical about the use of paragraphs for line breaks.
Contributor
Author
There was a problem hiding this comment. that's how CkEditor creates line breaks internally, we can reconfigure it to
Contributor
Author
There was a problem hiding this comment. I couldn't find docs for ckeditor5 but if you inspect composer and press enter you can see it live |
||
| const viewFragment = editor.data.processor.toView(htmlContent) | ||
| const modelFragment = editor.data.toModel(viewFragment) | ||
| editor.model.insertContent(modelFragment) | ||
| } | ||
| } else { | ||
| const itemElement = writer.createElement('paragraph') | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the logic behind this? I have created a test text block with both types of line breaks: a regular one and a paragraph. The database
contentis this:<p style="margin:0;">Paragraph 1 Line 1<br>Paragraph 1 Line 2</p><p style="margin:0;">Paragraph 2 Line 1</p>.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text is created in Ckeditor intially in the text block config so it will look like
<p> line one </p> <p> line 2</p>then on insert it will get converted to
line one\nline 2which here will get reconverted to
<p> line one </p> <p> line 2</p>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test with this input in the text block:
Then you will have two paragraphs. The first one has a line break. Do you still have this format after converting to line breaks and back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that works, because the intermediate to Plain conversion would have
\nfor both paragraphs and<br>would look like thisa\nb\ncThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visually it's the same, but it would only create
<p>which is okay imo for plain text, because at the end of day the html wont be sent and visually it's exactly the sameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect
\nfor a<br>and\n\nfor a<p>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a separate bug in https://github.com/nextcloud/mail/blob/fix/text-block-multiline/src/util/text.js#L96
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough