fix(markdown): don't always force line break

civing commented 3 weeks ago

If the newline character are preceeded with a backslash, don't force a hard line break since that combination already symbolizes a line break with the commonmark specification.

The markdown viewer are defaulting to force hard line breaks by replacing new lines (either \n or \r) with " \n". Two spaces according to markdown syntax followed by a new line characters will render as a new line withing a block. The purpose of this is to replace new line breaks with a markdown specific new line.

However, when done in combination with an actual line break using a backslash things get weird. '\\n' will be replaced by '\ \n' which will then be rendered as both a backslash and a new line in markdown. This is not the intention of the original markdown.

fixes: Lundalogik/crm-feature#4175

You can test this by entering the follow in the limel markdown example.

# Hello, world!

This is **markdown**!\
\
\
\
testing

This should render as new lines and remove the backslash.

Review:

[ ] Commits are atomic
[ ] Commits have the correct type for the changes made
[ ] Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

[ ] Chrome
[ ] Edge
[ ] Firefox

Linux:

[ ] Chrome
[ ] Firefox

macOS:

[x] Chrome
[ ] Firefox
[ ] Safari

Mobile:

[ ] Chrome on Android
[ ] iOS

github-actions[bot] commented 3 weeks ago

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3024/

adrianschmidt commented 3 weeks ago

I've tried to test this in the Text editor in markdown mode example in the PR docs, but I can't see how the PR fixes anything?

If I enter the text suggested, it of course renders backslashes, as I've written backslashes… If I just enter line breaks, those are removed from the output… 🙁

LawrenceBorst commented 3 weeks ago

I've tried to test this in the Text editor in markdown mode example in the PR docs, but I can't see how the PR fixes anything?

If I enter the text suggested, it of course renders backslashes, as I've written backslashes… If I just enter line breaks, those are removed from the output… 🙁

I had to add double slashes, since Javascript interpreter simplifies strings like

`test
\
\
\
test
`

to testtest, using \ to show newlines only aesthetically in the code, but not the string itself.

So I changed it to

`test
\\
\\
\\
test
`

But was this the problem? I didn't fully understand your comment.

adrianschmidt commented 3 weeks ago

But was this the problem? I didn't fully understand your comment.

I guess what I meant to say was that I am missing steps to reproduce the original bug, and I can't figure out how to reproduce it on my own.

civing commented 3 weeks ago

What's fixed here is to honor the hard brakes with a backward slash in markdown. This is one of the alternatives of doing a hard break for a new line very similar to two spaces. https://spec.commonmark.org/0.31.2/#hard-line-breaks

You can reproduce this if you go to either dev-feature or the markdown prosemirror example and pressing Shift-Enter in between two lines. Prosemirror markdown will then insert \\\n to represent a hard line break similar to the html <br>

This is one example I just created with Shift-Enter: https://dev-feature.lime-crm.com/client/object/company/16194?tab=_activities

Edit: this is the resulting history note that's created. Notice the lack of \ https://dev-feature.lime-crm.com/client/object/history/8397

civing commented 3 weeks ago

Sorry for the confusion regarding reproducing this. I believe I forgot to save the video explaining this to the linked issue but it should be there now. I hope things are clearer now.

civing commented 3 weeks ago

I've tried to test this in the Text editor in markdown mode example in the PR docs, but I can't see how the PR fixes anything?

If I enter the text suggested, it of course renders backslashes, as I've written backslashes… If I just enter line breaks, those are removed from the output… 🙁

Are you not getting the same line breaks as I do? 🤔

adrianschmidt commented 3 weeks ago

Ah! OK. Then I understand. Nothing I read said to press shift+enter, it just said to:

You can test this by entering the follow in the limel markdown example.
# Hello, world!

This is **markdown**!\
\
\
\
testing
This should render as new lines and remove the backslash.

I guessed that I wasn't meant to actually write backslashes, but I couldn't figure out what I was supposed to do.

Pressing shift+enter between two lines of text, and then switching over to read-only mode gives me the described bug when done on main, and that's been fixed in this PR. Nice! 👍

adrianschmidt commented 3 weeks ago

I'm not sure if "don't always force line break" is a very helpful description of what this change does, but I didn't want to spend too much time on coming up with something better and then waiting for it to build again. For next time though 😉

lime-opensource commented 3 weeks ago

:tada: This PR is included in version 37.49.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

Lundalogik / lime-elements

fix(markdown): don't always force line break #3024

Review:

Browsers tested: