Various fixes needed whilst using this in matrix-org/matrix-react-sdk's Rich Text Editor

tommoor / slate-md-serializer

A Markdown serializer for the Slate editor framework

MIT License

64 stars 36 forks source link

Various fixes needed whilst using this in matrix-org/matrix-react-sdk's Rich Text Editor #11

Closed ara4n closed 6 years ago

ara4n commented 6 years ago

Adds \n\n after paragraphs, blockquotes and lists (when needed) in order to correctly terminate them
Ensures blockquote correctly quotes all of its children, not just the first line (likewise for ATX headers)
Fixes a warning in latest immutable to use .size rather than .length

tommoor commented 6 years ago

Hey! Thanks for pushing this back upstream - the second two changes look like easy merges but I'm concerned by the introduction of new paragraphs.

The main current usecase for this serializer is for use with https://github.com/outline/rich-markdown-editor which purposefully maintains newlines instead of collapsing them like markdown typically would.

It would help if you include some test cases for the type of input you're protecting against here…

ara4n commented 6 years ago

ah, okay - i assumed it must be something like that, otherwise it'd be a pretty nasty omission. conversely we picked plain commonmark for Matrix, which collapses newlines. I'm still working out a few other related issues; sounds like this will need both UTs and a dialect options flag in order to fly? or is there another fork I should be PRing against?

(p.s. hi - thanks for writing this! we chatted at some webrtc conf in the US back when you were sqwiggling :)

tommoor commented 6 years ago

The reasoning for storing and parsing all newlines is that we felt it is important that what you see in the Slate editor is what ends up getting stored. If the two differ then saving and reloading will result in a different output.

This behavior changed in v3.0, so you could potentially fork from v2.0 and apply the few bug fixes that have happened since then if you prefer?

ara4n commented 6 years ago

Right, understood. we're only using the serialiser currently, and yup, these changes almost certainly break roundtripping (unless they were also implemented on the deserialiser). We'd rather base our fork off the latest version so we can keep up-to-date, and potentially contribute back other fixes (e.g. https://github.com/matrix-org/slate-md-serializer/commit/f7c4ad394f5af34d4c623de7909ce95ab78072d3#diff-5326222f837d36fd2bff476584c07621 feels like a thinko which would impact you too?).

Would you be amenable to just keeping the newline changes behind a config option (i.e. hand the serialiser a "generate commonmark" flag?)

tommoor commented 6 years ago

I was literally just looking at https://github.com/matrix-org/slate-md-serializer/commit/f7c4ad394f5af34d4c623de7909ce95ab78072d3#diff-5326222f837d36fd2bff476584c07621 separately - I created a failing test as someone reported it in our Outline community.

I might cherry pick that change across for a quick fix - I'd be open to flag for the commonmark yep but it needs associated tests.

tommoor commented 6 years ago

I'd love to accept these patches but as this PR stands I can't, closing for now.