Rosey/markdown-draft-js

preserveNewlines off by one newline

Closed this issue · 13 comments

danielsnider commented

Hi @Rosey,

I am really happy for the preserveNewlines feature. It got me closer to 1-to-1 exact matching looks when converting, but I found newline handling was still a little bit off. I was able to fix it without breaking any unit tests. My draft-js editor is pretty much this, the standard Rich Text Editor Draft.js example.

Here is my example:

Input:

a
b

c


d

When converted by draft-to-markdown.js it added a newline after each character, output:

a

b


c



d

And here's the same example but converted by markdown-to-draft.js, input:

a
b

c


d

When converted by markdown-to-draft.js it removed one newline after each character (except "a"), output:

a
b
c

d

Does this make sense?
Should I write unit tests to test these things?

I wrote a fix PR (#112)
Can this be included in your package and put into NPM to help us develop: rareconnect.org?

Rosey commented

Thanks! Tests would be really awesome too... The newline preservation behaviour is a bit of a tricky one, lots of edge cases, I also have this issue:

#108

Question for you: Draft by default does a "hard" newline when you press return, so a brand new paragraph, although you can optionally insert a soft newline. I'm wondering if the behaviour you're describing is expected given draft's default hard vs soft newline behaviour, but I'm not 100% sure as I haven't dug in to your solution or the issue too deeply yet, just read your note 😳

  • 👍1
danielsnider commented

I think I'll write new tests to help confirm if my issue is real.

Hopefully I don't have to modify soft vs hard new lines. I worry that I don't understand the subtleties around changing that. Hard newlines are <p> and soft newlines are <br>?

danielsnider commented

I ran my new tests on master without my changes, and they failed in the same way I described in my first post. I think this means it's not just me–that my issue is legitimate. Take a look at my new committed tests af777bd. What do you think?

Here's what it looks like when my new tests fail:

dan@ubuntu:~/markdown-draft-js$ npm test

> markdown-draft-js@2.1.1 test /home/dan/markdown-draft-js
> karma start --single-run

Deprecated private createPreprocessor() API
15 01 2020 14:26:15.867:INFO [framework.browserify]: bundle built
15 01 2020 14:26:15.873:INFO [karma-server]: Karma v4.3.0 server started at http://0.0.0.0:9876/
15 01 2020 14:26:15.874:INFO [launcher]: Launching browsers Chrome with concurrency unlimited
15 01 2020 14:26:15.879:INFO [launcher]: Starting browser Chrome
15 01 2020 14:26:16.748:INFO [Chrome 79.0.3945 (Linux 0.0.0)]: Connected on socket IlbwJ8IX6d9aV89wAAAA with id 52201770
Chrome 79.0.3945 (Linux 0.0.0) markdownToDraft renders unstyled blank lines correctly FAILED
  Error: Expected 'c' to equal ''.
      at <Jasmine>
      at UserContext.<anonymous> (/tmp/test/markdown-to-draft.spec.js:39:45 <- /tmp/76393500ce65b93ee2cb2ca479625b23.browserify.js:7752:45)
      at <Jasmine>
  Error: Expected '' to equal 'c'.
      at <Jasmine>
      at UserContext.<anonymous> (/tmp/test/markdown-to-draft.spec.js:41:45 <- /tmp/76393500ce65b93ee2cb2ca479625b23.browserify.js:7754:45)
      at <Jasmine>
  Error: Expected 'd' to equal ''.
      at <Jasmine>
      at UserContext.<anonymous> (/tmp/test/markdown-to-draft.spec.js:43:45 <- /tmp/76393500ce65b93ee2cb2ca479625b23.browserify.js:7756:45)
      at <Jasmine>
  TypeError: Cannot read property 'text' of undefined
      at <Jasmine>
      at UserContext.<anonymous> (/tmp/test/markdown-to-draft.spec.js:45:39 <- /tmp/76393500ce65b93ee2cb2ca479625b23.browserify.js:7758:39)
      at <Jasmine>
Chrome 79.0.3945 (Linux 0.0.0) draftToMarkdown whitespace handles unstyled blank lines FAILED
  Expected 'a
  
  b
  
  
  c
  
  
  
  d' to equal 'a
  b
  
  c
  
  
  d'.
      at <Jasmine>
      at UserContext.<anonymous> (/tmp/test/draft-to-markdown.spec.js:67:24 <- /tmp/76393500ce65b93ee2cb2ca479625b23.browserify.js:6228:24)
      at <Jasmine>
Chrome 79.0.3945 (Linux 0.0.0): Executed 98 of 99 (2 FAILED) (skipped 1) (0.255 secs / 0.094 secs)
TOTAL: 2 FAILED, 96 SUCCESS
npm ERR! Test failed.  See above for more details.

Thanks a lot for the help,
Daniel

danielsnider commented

Hi Rosey!

I hope you and your family are well!! No rush (family is more important) but we are waiting to hear if you have feedback or want to merge this PR (#112).

All the best!
Thanks again for this wonderful package!

Rosey commented

I'm sorry :) we've all been down with the flu this past week and are still on the recovery! Tomorrow is doctors appointments but I'll try to find some time later this week when the toddler is in daycare to sit down and look at this 🙂

danielsnider commented

Get well soon! 😊

Rosey commented

Ugh I'm actually back at the hospital again so depending on how things go it may be a longer wait :(

Rosey commented

Sorry things have been very busy around here. I'm just taking a quick look at the PR now but I'll probably have to finish looking a bit later. I think it looks good and thanks for writing tests! I just want to check your branch out locally and confirm that it still works correctly in cases where there's a blockquote followed by a paragraph, just because the comment in the code about "styled followed by unstyled" makes me wonder if in some cases an extra newline was necessary to avoid things like block quotes persisting, eg:

> Test
Hello

renders as

Test
Hello

and so if we were converting we'd want it to be

> Test

Hello

To ensure blockquote is escaped.

Anyway I'll confirm when I have a chance and write a test for this case if one doesn't exist!

danielsnider commented
Rosey commented

Closing as the related PR has been merged 🙂

Rosey commented

@danielsnider I released your fix under 2.2.0 🙂 👏 🎉

danielsnider commented

Thank you! 🎉🎉

danielsnider commented

@Rosey It was a pleasure to help. Thanks again for this great project.