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 70 forks source link

Pass options to remarkable #22

Closed juliankrispel closed 7 years ago

juliankrispel commented 7 years ago

Would be nice to pass down the options to Remarkable to have more control :)

juliankrispel commented 7 years ago

My use case for example is to parse html, and I prefer this converter over the others (was about to write my own when I found this)

thanks for the awesome work <3

Rosey commented 7 years ago

Thanks very much for the kind words and for contributing :) Also I can't believe I wasn't already passing options in, it seems like a no-brainer thing that should be added. I had the same frustration with existing draftjs markdown converters, needed more customization than they would easily allow.

One change though maybe? Instead of passing in options wholesale like this, since potentially markdownToDraft could also be accepting its own options and I don't want to get painted into a corner where remarkable options and markdownToDraft options accept the same key with a different meaning and get people all confused, I'm wondering if we can change this to

const md = new Remarkable(options.remarkableOptions);

This would also kind of follow the naming convention that's already one line below that, where we reference options.remarkablePlugins :)

Also, if you have the time, updating the README to include this little factoid, but if not I can do that in a separate PR later.

juliankrispel commented 7 years ago

Will do! Thx for the review 🙌 On Wed, Sep 20, 2017 at 4:38 PM Rose notifications@github.com wrote:

Thanks very much for the kind words and for contributing :) Also I can't believe I wasn't already passing options in, it seems like a no-brainer thing that should be added. I had the same frustration with existing draftjs markdown converters, needed more customization than they would easily allow.

One change though maybe? Instead of passing in options wholesale like this, since potentially markdownToDraft could also be accepting its own options and I don't want to get painted into a corner where remarkable options and markdownToDraft options accept the same key with a different meaning and get people all confused, I'm wondering if we can change this to

const md = new Remarkable(options.remarkableOptions);

This would also kind of follow the naming convention that's already one line below that, where we reference options.remarkablePlugins :)

Also, if you have the time, updating the README to include this little factoid, but if not I can do that in a separate PR later.

— 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/22#issuecomment-330891671, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIhWoCqoRqQioQ-0_d5nabMyiwpVJX7ks5skTFSgaJpZM4PdoKv .

juliankrispel commented 7 years ago

@Rosey done, lmk if all good.

Rosey commented 7 years ago

one small comment but looks good otherwise :) thank you! Do you think you could squash any pr-comment related fixup commits together as one and force push before I merge? (Just because I don't think we need 3 separate commits for this change 🙃)

juliankrispel commented 7 years ago

@Rosey I had the same idea :D

Rosey commented 7 years ago

Yay awesome thank you. All merged. Will draft a release shortly!

Rosey commented 7 years ago

or do a release 🤔 I guess draft implies it won't actually be released. I will do a release shortly.

juliankrispel commented 7 years ago

@Rosey sweeeet! Thanks for the swift response! Btw, in case you haven't and fancy some draft-js company, join us on slack <3 - https://draftjs.herokuapp.com/

Rosey commented 7 years ago

I'm in there! I'm not very active though, more of a lurker mostly 😅