Rosey / markdown-draft-js

A tool to convert content created in DraftJS to markdown and vice versa.
https://rosey.github.io/markdown-draft-js/
MIT License
318 stars 69 forks source link

Off by one fix for preserveNewlines #112

Closed danielsnider closed 4 years ago

danielsnider commented 4 years ago

To fix issue #111

Rosey commented 4 years ago

Just been testing a bit 🙂 It seems mostly good, I'm a bit nervous about the addition of + 1 and some of the logic changes because I don't fully understand the change I think 😳 I wrote this test, which passes in master but fails here -

  it('renders blockquotes correctly', function () {
    var markdownString = '> Hello I am Blockquote\n\nI am not\n\n> I am';
    var draftJSObject = markdownToDraft(markdownString, {preserveNewlines: true});
    var markdownFromDraft = draftToMarkdown(draftJSObject, {preserveNewlines: true});
    expect(markdownFromDraft).toEqual(markdownString);
  });

An extra newline is being added in this branch which throws off the idempotency -

Screen Shot 2020-01-28 at 9 04 38 AM
danielsnider commented 4 years ago

Thank you for the investigations! I'll try to improve the +1. All the best!

On Tue, Jan 28, 2020 at 12:05 PM Rose notifications@github.com wrote:

Just been testing a bit 🙂 It seems mostly good, I'm a bit nervous about the addition of + 1 and some of the logic changes because I don't fully understand the change I think 😳 I wrote this test, which passes in master but fails here -

it('renders blockquotes correctly', function () {

var markdownString = '> Hello I am Blockquote\n\nI am not\n\n> I am';

var draftJSObject = markdownToDraft(markdownString, {preserveNewlines: true});

var markdownFromDraft = draftToMarkdown(draftJSObject, {preserveNewlines: true});

expect(markdownFromDraft).toEqual(markdownString);

});

An extra newline is being added in this branch which throws off the idempotency -

[image: Screen Shot 2020-01-28 at 9 04 38 AM] https://user-images.githubusercontent.com/1326431/73286912-3efae880-41ad-11ea-89a8-8cfeda2a4978.png

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Rosey/markdown-draft-js/pull/112?email_source=notifications&email_token=AANIGWEORFAOFMODCFMKNX3RABQVNA5CNFSM4KGJ56D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKEDXNI#issuecomment-579353525, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANIGWGYOZEXZ5LNIVM6SVTRABQVNANCNFSM4KGJ56DQ .

Rosey commented 4 years ago

Thanks! Even if that +1 does make sense it may just be that I don't fully understand it, so an explanation might be all that is required, I don't know :) the newline stuff is tricky, since markdown doesn't technically support whitespace like this under normal circumstances

danielsnider commented 4 years ago

Hi @Rosey, I hope you and yours are well. Checkout my commit messages for brief explanations. Let me know what you think. Maybe it all makes sense or maybe we should audio chat to help reach a common understanding.

I'm wondering is it even possible to preserveNewlines with blockquotes and maintain idempotency? I don't think so. This test shows why:

  it('fails to be idempotent for blockquotes and preserveNewlines', function () {
    var markdownString = '> Hello I am Blockquote\nI am not';
    var draftJSObject = markdownToDraft(markdownString, {preserveNewlines: true});
    var markdownFromDraft = draftToMarkdown(draftJSObject, {preserveNewlines: true});
    expect(markdownFromDraft).toEqual(markdownString);
  });

image

Rosey commented 4 years ago

Thanks very much for all your work on this, that makes sense 🙂