Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate rich text editor to Slate backed by Unified #254

Merged
merged 79 commits into from
Aug 25, 2017

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Mar 2, 2017

fixes #246

- Summary

Currently netlify-cms uses three different libraries for handling Markdown. markup-it for the render preview, markdown-it transforming markdown src to Prosemirror's document model, and finally Prosemirror's internal deserialization from their internal model back to Markdown.

There's been several inconsistencies that develop because of this. Also using multiple libraries raises the barrier to contributing bug fixes and new features.

This WIP PR consolidates all of these on the Remark library as it exposes a very nice AST for Markdown that makes it easy to do the conversions we need.

What's done currently is switching in Remark for markup-it for rendering markdown plus most of the compilation process from markdown source to the prosemirror document model.

So that part needs finished plus writing the deserialization method for converting prosemirror to remark to raw markdown. Also looking into the paste from dropbox problem (not related to the markdown conversion though so could be pushed into a different PR).

Also need to restore plugin support which I'll want to talk to you about @biilmann as not entirely sure how that should work.

@biilmann
Copy link
Contributor

biilmann commented Mar 3, 2017

Very promising!

One thing I noticed is that it looks like we're missing the "tight" parameter for lists somewhere.

Try going to https://deploy-preview-254--cms-demo.netlify.com and edit the first blog post. Double click to select a word and click the <> icon to switch back and forth between raw mode and visual. Now each bullet in the list has a blank line between them.

In the current version I worked around this by modifying ProseMirror's example Markdown parser to always set the tight attribute on lists:

https://github.com/netlify/netlify-cms/blob/master/src/components/Widgets/MarkdownControlElements/VisualEditor/parser.js#L234

@KyleAMathews
Copy link
Contributor Author

Cool! Yeah Remark has that attribute too I noticed — just need to copy it over I guess.

@KyleAMathews
Copy link
Contributor Author

Ok pushed another commit supporting the remaining block elements + adding support for parsing inline markdown elements into the Prosemirror schema. Fenced code blocks w/ languages are now correctly handled (apparently Prosemirror always handled it but the default markdown parser didn't add the language attribute ProseMirror/prosemirror#272 (comment)).

@KyleAMathews
Copy link
Contributor Author

Also just tested html in code and it works just fine now.

@KyleAMathews
Copy link
Contributor Author

What was the problem with nested lists? I did a bit of testing and they seem fine.

Also since issues with pasting from Dropbox is a separate issue, perhaps let's merge this and work on that in a separate issue?

@KyleAMathews
Copy link
Contributor Author

TODOs here that I could use help with are handling plugins + drag-n-dropped images as I don't entirely understand what's intended there.

@biilmann
Copy link
Contributor

For plugins:

If you go to the in-memory demo and use the editor, there's currently a "Youtube plugin" defined by the fictional user.

https://cms-demo.netlify.com/#/collections/posts/entries/2015-02-14-this-is-a-post

When you have the cursor on a new line where you can add a new block level element, you'll get the option of inserting a new Youtube embed:

screen shot 2017-03-21 at 2 25 26 pm

The example component is defined here: https://github.com/netlify/netlify-cms/blob/master/example/index.html#L132

In this case adding a youtube embed will insert a hugo style shortcode, but it could just as well be an HTML snippet, a JSX component, or whatever the static site generator building the content wants to process.

In the current version of the editor, the inserted component is an opaque bloque, and you can't edit it once inserted. Ideally clicking the box should open up the same form used to create it so you can edit the values.

There's some more context around this in my comments on the RTA design issue #288

For drag and dropped images, we currently use the addAsset method passed through the props to make sure the CMS knows this asset needs to be persisted together with the entry, and then insert a normal markdown image tag based on the image in the editor.

@erquhart
Copy link
Contributor

@KyleAMathews awesome work on this man. I'm considering taking a step back in our trajectory for the editor and I'd like your input. So here's my thing:

I'm not convinced we need to work as hard as we are, and I'm looking to avoid the overhead of maintaining so much editor customization for Netlify CMS contributors.

I've got my eye on Ian Storm Taylor's new-ish Slate framework. I see a lot of benefits, but I'm curious if you've seen this particular library and your thoughts on using it, given all of the time you spent digging into the RTE.

Thoughts?

cc/ @biilmann

@biilmann
Copy link
Contributor

No matter which editor framework we do, we'll want to put quite a bit of work into this component to get things like editorComponents to work etc.

If you dig through the early commits you'll see we initially started out using Slate as the backend for the editor component, but had to scratch it because the core editor performance got unworkably slow for larger documents.

Prose mirror and slate are very similar architecturally (both have an internal immutable document model, and requires some form of serialization/deserialization back and forth between the final output and the editors internal representation). The author of ProseMirror have managed to get corporate sponsors for a lot of his work and it's progressing really well for that reason, so I think it'll turn out to be the best choice also in the longer run...

@KyleAMathews
Copy link
Contributor Author

KyleAMathews commented Mar 31, 2017

Perhaps just reach out to Ian and chat with him? He's in SF. I'm sure would be incredibly interested in making sure Slate works really well for the Netlify CMS in the short and long term.

@erquhart
Copy link
Contributor

erquhart commented Mar 31, 2017

@KyleAMathews nice! Thanks for pointing that out.

@biilmann good points, can't go wrong with Marijnh + corporate sponsors. It seems on Slate's demo site that it still may have issues with larger documents.

@erquhart
Copy link
Contributor

erquhart commented Apr 4, 2017

@KyleAMathews somehow I missed your more recent updates - didn't realize this is more or less merge ready, very solid improvements!

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KyleAMathews
Copy link
Contributor Author

@erquhart I think it needs support for addAsset still perhaps?

@KyleAMathews
Copy link
Contributor Author

Oh actually nvm, doesn't look like anything changed there.

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize editorComponents was out, so we've still some work to do here.

package.json Outdated
@@ -9,7 +9,6 @@
"test:watch": "jest --watch",
"build": "NODE_ENV=production webpack --config webpack.prod.js",
"build:scripts": "NODE_ENV=production webpack --config webpack.cli.js",
"prepublish": "npm run build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleAMathews guessing this was meant to be added back in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh... maybe? I find prepublish scripts like this super annoying. But I can add it back.

Copy link
Contributor

@erquhart erquhart Apr 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on the annoyance front, but publishing a broken build is worth avoiding. We did recently drop the precommit hook, which should take the annoyance factor down by an order of magnitude.

this.plugins = {};
// TODO add back support for this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed this

import { EditorState } from 'prosemirror-state';
import { EditorView } from 'prosemirror-view';
import history from 'prosemirror-history';
import React, { Component, PropTypes } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing a package update is causing our linter to require double quotes?

@KyleAMathews
Copy link
Contributor Author

Just got back from a trip — will finish this up in next day or two.

@erquhart
Copy link
Contributor

Thanks @KyleAMathews - hope you had a good trip! The primary concern here is ensuring against breaks to existing functionality such as editorComponent. Excited about uncomplicating our serialization with Remark.

@KyleAMathews KyleAMathews force-pushed the cerealize branch 2 times, most recently from c635fb4 to fe40c01 Compare April 14, 2017 17:31
@KyleAMathews
Copy link
Contributor Author

Could someone explain the relationship between plugins and tokens? In this line https://github.com/netlify/netlify-cms/pull/254/files#diff-5d32b738f6c2db4fdf6fce8e1c55e110L103

It seems like plugins are relying on the token names provided by the default ProseMirror markdown parser?

@KyleAMathews
Copy link
Contributor Author

If so, they'd need to be updated to rely on the AST provided by Remark.

@erquhart
Copy link
Contributor

@KyleAMathews I'd have to dig in to understand that part myself. I'll have a look soon if no one else beats me to it.

@jholmes033169
Copy link
Contributor

I know this is sort of at the end of this PR lifecycle, but I was wondering where this issues falls within the overall scope. Generators like Hexio.io use "tag plugins" to extend markup, which the current solution breaks (they get rewritten as "code"). Also, the same generator allows HTML, which also gets rewritten.

Does this PR address any of those issues or offer a path to resolving them? Basically, need a "don't clean this, just pass it through" switch.

@erquhart
Copy link
Contributor

@jholmes033169 this branch does run - pull it down and try it out for your use case. I don't know much about Hexo (yet), but we do want max compatibility.

@erquhart
Copy link
Contributor

@KyleAMathews the tokenization is more related to our use of MarkupIt, but the plugin tokens are created in the CMS - see the augmented syntax function here: https://github.com/netlify/netlify-cms/blob/e66b3597ef7b67862705f1e6f42b9af8f82162c2/src/components/Widgets/richText.js#L27

@calavera calavera self-requested a review April 19, 2017 01:24
Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I don't forget to double check this before it's merged. Please, make sure the branch is properly rebased and there are no extra commits that don't belong to this change.

@calavera
Copy link
Contributor

This is awesome @erquhart. Good job 💪

@erquhart erquhart merged commit 79c30b9 into decaporg:master Aug 25, 2017
erquhart added a commit that referenced this pull request Sep 6, 2017
erquhart added a commit that referenced this pull request Sep 7, 2017
erquhart added a commit that referenced this pull request Sep 8, 2017
dopry pushed a commit to dopry/netlify-cms that referenced this pull request Sep 11, 2017
Benaiah pushed a commit that referenced this pull request Sep 15, 2017
erquhart added a commit that referenced this pull request Sep 18, 2017
erquhart added a commit that referenced this pull request Sep 18, 2017
erquhart added a commit that referenced this pull request Sep 18, 2017
erquhart added a commit that referenced this pull request Oct 6, 2017
erquhart added a commit that referenced this pull request Oct 9, 2017
erquhart added a commit that referenced this pull request Oct 10, 2017
erquhart added a commit that referenced this pull request Oct 17, 2017
erquhart added a commit that referenced this pull request Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Markdown serialization/deserialization
7 participants