Markdown Gutenberg block for Jetpack #9705
Conversation
Ferdev
added
[Status] In Progress
[Focus] Blocks
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
from
67047ce to
ad99ff1
Compare
brbrr
added
the
[Type] Enhancement
| category: 'formatting', | ||
|
|
||
| attributes: { | ||
| //The Markdown source is saved in the block content comments delimiter |
There was a problem hiding this comment.
What was your reasoning around saving the source in the attributes? Why not in the block's body? Why not in block meta?
There was a problem hiding this comment.
I thought it was the cleanest solution. It makes super simple to link the markdown source with the block's content (in case you add more than one markdown blocks).
When making blocks I guess we should strive to save the attributes in the block's content, that way we avoid data duplication, but I couldn't find a way to store the markdown source in the block's content and then hide it in the published post (but I'm a total WordPress newbie, maybe there's a way)...
Why not in block meta?
I guess you meant in the post's meta, right? I guess one of the uses of the post's meta is to share post's info in another parts of WordPress: other plugins, themes, etc. I didn't think the markdown source would be needed in any other place. It would make it harder to manage each markdown block source: if we store the attribute as it is, it'll return a strings array. If we choose to store as json, it'd have to serialize/parse it every time we needed. It seemed way more complex to do it this way.
There was a problem hiding this comment.
At a glance, I agree with keeping the Markdown source as a regular (HTML-comment-borne) attribute. Aside from ease on the development side, it also gets us data deduplication for free (that is, when rendering the front-end, assuming Gutenberg is stripping HTML comments, the visitor only gets the generated HTML source).
For completion:
-
Keeping the Markdown source in the block's body could be achieved using old techniques such as using custom script tags like
<script type="text/markdown">that the browser can ignore. Gutenberg can be told how to source attributes from these. The downside is that this information is wastefully sent to the visitor. -
It's possible to intercept the block's body to decide what gets sent with the rendered page, but that requires making the block dynamic (and having a PHP render callback to render the block on every page load). I don't see how this option could be better than defaulting to HTML comments for the Markdown source, though.
There was a problem hiding this comment.
Thanks for the clarification @mcsf ! Especially the last point 👍
package.json
Outdated
| @@ -63,6 +63,7 @@ | |||
| "classnames": "2.2.1", | |||
| "click-outside": "^2.0.2", | |||
| "clipboard": "^1.7.1", | |||
| "commonmark": "^0.28.1", | |||
There was a problem hiding this comment.
Also, do you think we should pin the version here?
There was a problem hiding this comment.
Why
commonmark?
I thought using a Markdown parser library made more sense than doing it myself: that could be easily a different project since it isn't something trivial to develop.
I used a commonmark implementation for Elixir in the past, I knew it was fast and reliable. Also, I asked in Slack's #javascript channel and was one of the recommendations. This javascript implementation project is active and well documented, so it was my first choice. Now I've found it lacks one feature and I've switched to markdown-it (which is another implementation of the commonmark specification) in my latest work: the parser does not store the delimiters used in the source, which I use for the live preview part of the project.
Also, do you think we should pin the version here?
I totally missed this one. I usually use npm to manage my node dependencies, and I had the npm config set save-prefix='~' enabled in my local npm config since a long time ago. Yarn doesn't seem to use this setting, and it used the ^ to save the package. I like to use ~ because that way only the patch part is updated, which means any bugs would be fixed in further updates. But maybe you have another policy in Automattic and you're more conservative in this aspect (which I totally understand) and you always use fixed versions of packages. Anyway, thanks for pointing this out!
| } = window.wp.element; | ||
|
|
||
| const markdownReader = new commonmark.Parser(); | ||
| const markdownWritter = new commonmark.HtmlRenderer(); |
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
from
3e6c450 to
5dbe189
Compare
| description: [ | ||
| __( 'Write your content in plain-text Markdown syntax.' ), | ||
| ( | ||
| <p> |
There was a problem hiding this comment.
I see mixed usage of JSX and el() notation in this file.
There was a problem hiding this comment.
Ops, my fault, first time using JSX. I'll fix that
| return <RawHTML>{ content }</RawHTML>; | ||
| } | ||
| } | ||
| export default MarkdownPreview; |
There was a problem hiding this comment.
This component could be a function.
| { placeholderSource } | ||
| </p>; | ||
| } | ||
| return <MarkdownLivePreview |
There was a problem hiding this comment.
I suggest tweaking the terminology somewhere. The distinction between "Preview" and "Live Preview" can be confusing.
There was a problem hiding this comment.
Totally agree. Actually, this was something I was planning to do because indeed the names are very similar and can be confusing.
| const placeholderSource = __( 'Write your _Markdown_ **here**...' ); | ||
|
|
||
| if ( ! isSelected && this.isEmpty() ) { | ||
| return <p className={ `${ className }-placeholder` }> |
|
|
||
| return [ | ||
| editorOrPreviewPanel.call( this ) | ||
| ]; |
There was a problem hiding this comment.
I don't see the need for editorOrPreviewPanel :). Is it an artefact of something? Furthermore, the array shouldn't be needed.
There was a problem hiding this comment.
Yeah, there's no need for this function anymore. The code inside the render function was more complex when the block had the "Editor" and "Preview" panels. But when I removed that feature I forgot to simplify this code and remove editorOrPreviewPanel
I'll fix that 👍
| }; | ||
| } | ||
|
|
||
| render() { |
There was a problem hiding this comment.
Convention has it that render should be the last method in the class body. Also, lifecycle methods should precede any custom methods:
- constructor
- componentDid… / shouldCompo…
- foo / bar
- render
There was a problem hiding this comment.
Thanks! 🙇♂️ I didn't know what the convention was.
| ); | ||
| } | ||
|
|
||
| shouldComponentUpdate( nextProps, nextState ) { |
There was a problem hiding this comment.
Thinking of the future, getSnapshotBeforeUpdate will probably be more apt for the caret position saving.
|
|
||
| const { source } = props; | ||
|
|
||
| setupMarkdownParser(); |
There was a problem hiding this comment.
This will be called on each instantiation of a preview — expectably, multiple times over the lifespan of the editor — thus setting up the same global every time. Seems we should avoid that.
There was a problem hiding this comment.
Yep, good catch, thanks! I'll fix that.
|
|
||
| const emptyState = '<p></p>'; | ||
|
|
||
| const emitChange = function( evt ) { |
There was a problem hiding this comment.
What was the rationale for keeping this function outside the component class?
There was a problem hiding this comment.
Independently of that, why isn't emitChange bound in the class constructor?
There was a problem hiding this comment.
Yet another question :) : why the name emitChange? To me it reads as if the function is doing more than it is, especially compared with some similar handlers found in core Gutenberg.
There was a problem hiding this comment.
Answering your first two questions: I guess this was just personal preference. I tend to not expose those functions that aren't supposed to be consumed out of the class. I don't know what is the convention you'd use here, but this was my reasoning.
And regarding the name... Yeah, probably that wasn't my best choice 😅 I'll give it another shot.
|
|
||
| const ignoreLastInput = function( source ) { | ||
| return endsWith( source, ' ' ) || endsWith( source, ' ' ); | ||
| }; |
There was a problem hiding this comment.
Can we represent the space characters differently to make it obvious what they are?
There was a problem hiding this comment.
Yeah, totally, this is something I had in my list of improvements. I was planning to create some constants using String.fromCharCode(0x00A0) to avoid mistakes between the two different space chars.
|
Thanks for your time in the review, @mcsf ! Just answered your questions, I'll push some commits tomorrow to tackle your suggestions & improvements. |
|
How is the live preview functionality meant to behave when I copy & paste MD into the block? I tried copying https://raw.githubusercontent.com/mxstbr/markdown-test-file/master/TEST.md into the block, and it didn't update until I pressed a key. Also, what subset of MD are we supporting? I can see some oddities when pasting Finally, what is the performance impact of live preview? What happens if we have lots of text? |
|
The problem you had pasting markdown code is a bug and it's weird because this was working... I'll perform a git bisect to check where the issue was introduced and, of course, fix it. It's supposed to live preview as you paste any markdown code. Yeah, I see those oddities too! The parser supports the Commonmark specification and the problem has something to do with how the text is pasted in the component and maybe some white-space issues too... I'll check that and will come back with an update. From the tests I've done, the performance is pretty good even for large texts in Chrome and Firefox, but in the list of pending things I have to perform proper performance tests. I was planning to do that when all features and bugs had been addressed. |
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
2 times, most recently
from
7e45cb0 to
0a98895
Compare
|
I've fixed the issues you had pasting Markdown code, @ehg. This has been addressed in ec681b3, 1b4022c and 30c5137. In the description of those commits you'll discover the reasons causing that problem. |
Yeah, this isn't ideal. Do you know why |
| render() { | ||
| const { ...props } = this.props; | ||
|
|
||
| return createElement( |
There was a problem hiding this comment.
Why are we using createElement here instead of JSX?
There was a problem hiding this comment.
This was just me using React and JSX for the first time. I'll change it to JSX notation 👍
| { | ||
| ...props, | ||
| ref: ( e ) => this.htmlEl = e, | ||
| onBlur: renderMarkdownPreview.bind( this ), |
There was a problem hiding this comment.
bind creates a new function on every render. The shouldComponentUpdate looks like it will make this less of a problem, but we should avoid doing this regardless as it can lead to unexpected behaviour.
There was a problem hiding this comment.
Yeah, you're totally right. Actually, I found another place where we're creating a new function quite often: here
I'll fix both issues 👍
package.json
Outdated
| @@ -116,6 +116,7 @@ | |||
| "json-loader": "^0.5.7", | |||
| "lodash": "^4.9.0", | |||
| "node-sass": "4.9.0", | |||
| "markdown-it": "~8.4.1", | |||
There was a problem hiding this comment.
Let's pin this to the exact version number, in case there's semver principles are skipped upstream :)
| @@ -10,7 +10,8 @@ const webpackConfig = { | |||
| // The key is used as the name of the script. | |||
| entry: { | |||
| admin: './_inc/client/admin.js', | |||
| 'static': './_inc/client/static.jsx' | |||
| 'static': './_inc/client/static.jsx', | |||
| 'modules-markdown-block': './modules/markdown/assets/js/jetpack-markdown-block.jsx', | |||
There was a problem hiding this comment.
Why do we need to add an entry point here? Can we get away without one?
There was a problem hiding this comment.
Allow me to investigate this further, please. I saw this was the pattern used in other PRs for Gutenberg blocks in Jetpack and I just thought it was a valid solution.
|
At first I was disappointed to see that you're implementing the Commonmark spec of Markdown instead of Markdown Extra as already exists in Jetpack. Yeah, I like footnotes... Then I realized that there are plugins to add footnote capabilities to markdown-it. Will you be adding one of these in the future? Please? ;-) |
Yeah, it's by design. The CommonMark Spec does not allow heading or trailing whitespace in paragraphs (see point 4.8 paragraphs here):
The problem is Markdown-It doesn't offer a setting to customize this, which I think is reasonable since the specification doesn't allow it. I think our options here are:
Maybe it's worth to invest more time in a deeper analysis of other options, and then make a final decision. What do you think? |
|
@brentlogan sure! One of the reasons for choosing Markdown-It is it makes easy to extend the specification with custom rules. Actually, the fact that it supports footnotes was a ➕ when I saw the classic editor supported them. Do you think we should implement any other rules not contemplated in the CommonMark spec? Thanks for your feedback! 🙇 |
|
@Ferdev I looked at Markdown Extra to look at its features that I regularly use:
I guess I'm a big fan of Markdown Extra... ;-) As best I can tell, the latter two are included in Commonmark. Thanks for listening! |
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
from
30c5137 to
975ebff
Compare
|
yeah @brentlogan , I'd say all your use cases are covered by Markdown-It. You can check its features in their live editor here 🎉 |
|
I've performed a more in-depth analysis of existing Javascript Markdown parsers to help us choose the most suited for our needs. The Markdown block uses the parser in two points: the Save component of the block, and the Live Preview component:
These are the most used Markdown parsers right now: showdownThis is probably the most adopted one. GoogleCloudPlatform, Ghost, Meteor... are some companies using it. Its engine is based in regular expressions and sequential parsing of the Markdown source. This makes difficult to disable parts of the specs, which we need to do for the Live Preview component. commonmark.jscommonmark.js is a lightweight Markdown parser based in the CommonMark specification. It generates an abstract syntax tree that can be traversed to perform some modification/customization to the resulting HTML. Unfortunately, it lacks some features like storing the token delimiters used by the author of the Markdown source to add them back in the live preview component. It's the fastest of all of them. MarkedMarked is in the analysis just because is one of the most used ones, but it turned out to be not very performant and also it lacks any form of extensibility. markdown-itmarkdown-it is the library being used in the Markdown block currently. The performance is excellent, and it allows easy extensibility, either using the existing plugins or adding custom rulers (this is what the Live Preview component makes use of). Follows the CommonMark spec. remarkableThe last one in the analysis is quite similar to markdown-it. Actually, the markdown-it developers are the authors of 99% of the Remarkable code, but they decided to create a new project (markdown-it) with new leadership. It's faster than markdown-it but has worst extensibility. PerformanceI've created a small jsperf test suite to measure the speed of each library in the analysis. How markdown-it engine works and extensibilityThe markdown-it engine has an ordered set of defined rules to parse each line in the Markdown source. The engine loops over each line, and applies each rule sequentially to the line of markdown source. If the rule is applied successfully, no more rules are evaluated over that line, and the engine keeps going with the next line. The API has methods to modify the ordered set of rules, so for example, a new paragraph rule could be evaluated before the existing one for all lines starting with upper case letters. This way you won't be replacing any existing rules. The Live Preview component takes advantage of this to rewrite the ConclussionAfter having analyzed the most used Javascript Markdown parsers, I'm quite happy with the current approach: leveraging on the extensibility of the markdown-it parser. The performance is very good, the parser API is very flexible, and the current specification (CommonMark) can be extended easily with plugins to support Footnotes, definition lists, and some other niceties. The other options are either less performant, or less customizable, so I really don't see any other option than creating our own parser. I'm pinging @dmsnell here because @ehg mentioned in Slack he could be interested in this. |
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
from
85e6ca6 to
5d2c9c8
Compare
oskosk
added
the
[Status] Needs Review
Updates the render method to use JSX notation instead of the Gutenber createElement API method. Also, removes some `bind` calls in the render method.
Every time the caret position was saved, a new instance was being created of the `restore` function inside the `saveCaretPosition` function of the caret-management library.
Adds a test to check if the block is being registered with the correct params.
Tests if the save component renders the markdown renderer.
Tests if the MarkdownRenderer component generates HTML from Markdown source successfully.
Adds a test to check if the MarkdownConverter utility renders the preview and the full version of the Markdown source properly.
Adds a test to check if the editor component renders the live preview component successfully.
Checks if the component renders HTML from a Markdown source, using a subset of the CommonMark specification.
Tests the change event handler and the CommonMark specification supported by the live preview plugin.
Ferdev
force-pushed
the
add/markdown-gutenberg-block
branch
from
5d2c9c8 to
c9be670
Compare
Adds javascript code in the Markdown block module to the `.jshintignore` file since all javascript code in this module is ES 6.
Instead of the live preview, a preview tab has been added to the Markdown block. When the user clicks it, the Markdown source is rendered as HTML. The Live Preview component development has been moved to the branch `update/markdown-gutenberg-block-live-preview`.
There was a problem hiding this comment.
Thanks for working on this @Ferdev
I've been watching this PR for a while and have been thinking about it often - I too have made a few attempts at building a Markdown block.
The challenge has never been technical - it has always been one of choice, raised in the discussion with @ehg and @mcsf about what to store where. At some point, as you discovered, we have to just make a decision and accept it. There are many strong benefits to storing the source content in the JSON attributes and render the HTML from the browser.
That being said I think one thing that is missing on this PR is a high-level design and exploration of the tradeoffs inherent in the different systems. The term "Markdown block" is somewhat ambiguous because it glosses over big design choices:
- is the purpose to write HTML using Markdown as a kind of shorthand to make it go faster?
- is the purpose to write HTML using Markdown as the "source" because it's easier for people to think about than raw HTML or styled text?
- what is the intended static render when everything else is gone? is it the rendered HTML from the Markdown source or is it the syntax-highlighted Markdown source itself?
- which "Markdown" are we supporting and how will we support non-standard features: Github-flavored Markdown with tables and highlighted code fences, straight-up HTML tags like
<abbr>and<nav>and<script>, etc… - using a renderer in the browser means this will behave differently than the existing Jetpack Markdown support; what do we make of that?
- what is our strategy for the duplication of content in the source vs. the render? how big of a concern is it?
- what does it mean to have a Markdown document in a block paradigm? do we split the Markdown block on every newline? do we create header blocks for every
#header?
there are quite a few further questions I'd like to raise in this PR but I think having a guiding design document might be what I consider the greatest need just because this is such a non-technical challenge. would you be willing to write up a kind of README for this with the tradeoffs explored and a summary of why we chose what we did? I think this would greatly help future developers who want to continue iterating on this block and will clarify what is and what isn't inherent to the meaning of the block itself.
Once that's taken care of I think we might be able to dramatically simplify the code in here.
Thanks for your work and your persistence! Please don't hesitate to ping me or ask clarifying questions!
oskosk
added
[Status] Needs Author Reply
|
Apologies for the delay in my reply, @dmsnell! Our family is moving from Spain to the USA this August and I have a lot on my plate right now. Thank you very much for showing your thoughts on this PR. I'm glad to see you are asking the same questions I asked myself when I started working on this. One of the things I missed after I checked the work already done on this feature, was some sort of documentation from a design standpoint. I was expecting those questions to be had been answered there. Another question I had was: where should be footnotes placed in a post with several different blocks? But in retrospective, I guess the most critical question that I asked myself and that I should've thrown in the open was: where does this functionality come from? I guess I was a bit confused because I didn't see a definite place where all design questions and feedback had been gathered and discussed. Have you ever considered using a Request For Comments methodology, or do you use any other system to do so? I'd love to talk more about this. I share your concern about the design problem this feature represents more than its coding counterpart. Actually, when I shared the first version with my assigned team, I asked for feedback from the designers more than anything. I think it's a great idea to put together all my thoughts and decisions on a README 👍. Just give me a couple of days to gather some time, and I'll push it to this PR. Thank you again for your feedback and the time spent thinking about all this, Dennis! 🙇 |
Adds the Jetpack Markdown block to the set of Gutenberg extensions, based on the previous work done by @Ferdev in: Automattic/jetpack#9705. Co-authored-by: Fernando Espinosa <[email protected]>
|
Since our plan is to build JS (and maybe CSS) within Calypso env - we shouldn't merge this PR as is. JS/CSS part already merged here: Automattic/wp-calypso#26336, the remainig part is to ship PHP code for this block. Moving this PR into next release. |
|
Code for the block has been merged in Automattic/wp-calypso#26336 and lives at https://github.com/Automattic/wp-calypso/tree/1b8afd08cc4427389ce5632aec97e4e19136db31/client/gutenberg/extensions/markdown currently. We'll need to figure out PHP part of this separately in a different PR. |
Changes proposed in this Pull Request:
Description
Creation of a Markdown Gutenberg block. The user will be able to enter Markdown code in Gutenberg, and it will be rendered into HTML in the client. The block will allow creating new content using Markdown, as well as editing existing Markdown blocks.
This PR comes from the previous work done by @nuzzio in the PRs #9357 and WordPress/gutenberg#6491 (thanks!) and addresses #9201. As a summary, a decision was made to perform the Markdown to HTML conversion in the client, which is the purpose of this PR.
The block will have the following features:
Basic text editing buttons to make text bold and italic, create links and add images. They should mimic the UX in the RichText Gutenberg editor component.Add a checkbox to allow the user disable the live-preview (for very big markdown sources).A Markdown syntax legend in the block description (for Markdown newbies).The last three features have been moved to PR #9825
The component will be developed following the order specified in the list above.
This PR notes will be updated as it evolves.
Testing instructions:
Steps to test the Markdown block component:
Posts can be edited as well.
The block Javascript code will be unit tested using the updated test setup in the PR #9687 that allows testing of Javascript code within modules.The block development will be test driven so the test suite will grow as the development progresses.Unfortunately, until issue WordPress/gutenberg#3955 is addressed, custom blocks developed outside Gutenberg cannot be unit tested due to the dependencies they have with Gutenberg core blocks and components.
Proposed changelog entry for your changes:
Added a markdown block for the Gutenberg plugin.
Checklist: