reconcile ckedidtor, showdown multiline list items #941
Conversation
| * linebreaks, the same as CKEditor produces. | ||
| * | ||
| */ | ||
| export default class MarkdownListSyntax extends MarkdownSyntaxPlugin { |
There was a problem hiding this comment.
@alicewriteswrongs You mentioned a few possibilities in slack
but anyway - we can make an intervention at a few points, [1] we could try to fix how Showdown handles this, or [2] we could try to edit and fix up it’s html output before it’s passed to ckeditor, or [3] we could try to see if there’s a way to make ckeditor tolerate
tags inside of list items
Since this is an "output" showdown rule, it basically seems like [2] to me. Using an output rule seemed more convenient than a "lang" rule since having the html tree is convenient for locating li > p.
I did not explore (3) too much beyond reading about the schema, and block elements not containing block elements and reading through some of ckeditor/ckeditor5#2973.
| filter: () => false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
hmm, if we wanted we could make these properties optional and change the parent class to check if they're present before calling them...idk, a bit annoying to have to declare a boilerplate rule that doesn't do anything
There was a problem hiding this comment.
Good point. How about changing the property to turndownRules: TurndownRule[]. That way:
- it's still required, so you have to actively opt-out rather than just forgetting to make the rule.
- but the array can be empty, so the boilerplate is minimal
- more symmetry between
turndownRulesandshowdownExtension, since the latter is already a list of showdown rules
There was a problem hiding this comment.
The only downside that I can see is a bigger diff / cluttering the git history due to formatting the existing rules as an array. Still seems like a reasonable solution, though, so I went with it.
There was a problem hiding this comment.
yeah that sounds like a good approach to me! arrays are nice for this kind of thing
| * - the HTML that CKEditor expects for lists | ||
| * | ||
| * When a markdown list item contains multiple lines, showdown puts <p> tags | ||
| * around all list items (and multipple <p> tags for the multi-line list item). |
There was a problem hiding this comment.
small spelling thing, s/multipple/multiple/
| } | ||
| this.setMarkdownConfig(newConfig) | ||
| } | ||
|
|
||
| abstract get showdownExtension(): () => Showdown.ShowdownExtension[] | ||
|
|
||
| abstract get turndownRule(): TurndownRule | ||
| abstract get turndownRules(): TurndownRule[] |
There was a problem hiding this comment.
maybe it's getting too fiddly, but I tested this out locally and it would work for allowing no boilerplate at all on subclasses of MarkdownSyntaxPlugin
| abstract get turndownRules(): TurndownRule[] | |
| get turndownRules(): TurndownRule[] { | |
| return [] | |
| } |
then basically subclasses that want to set a turndown rule would override this method, and others wouldn't. We could make the same change to 'de-abstract' the showdown method if we wanted as well
anyway - consider that a fully optional suggestion 😄
There was a problem hiding this comment.
I'm going to leave as-is for now because I like the idea that you have to opt-in to "no, I really don't need Turndown rules for this extension". Maybe that's silly—we would probably catch that sort of error in the unit tests 🤷
Adds a markdown "extension" that addresses an incompatibility between - the HTML that showdown produces for lists - the HTML that CKEditor expects for lists When a markdown list item contains multiple lines, showdown puts <p> tags around all list items (and multipple <p> tags for the multi-line list item). See https://github.com/showdownjs/showdown/wiki/Showdown's-Markdown-syntax#known-differences-and-gotchas. CKEditor, on the other hand, forbids paragraphs inside list items. Both are "block" elements in its schema; see https://ckeditor.com/docs/ckeditor5/latest/framework/guides/deep-dive/schema.html#defining-additional-semantics and block elements cannot contain other block elements. (There is discussion of having a list variant that can contain block elements; see ckeditor/ckeditor5#2973 for more.) So here we add a Showdown rule to replace list item paragraph tags with linebreaks, the same as CKEditor produces.
ChristopherChudzicki
force-pushed
the
cc/multiline-list-items
branch
from
ac462fe to
c46058c
Compare
Pre-Flight checklist
What are the relevant tickets? #935
What's this PR do?
Adds a markdown "extension" that addresses an incompatibility between
When a markdown list item contains multiple lines, showdown puts
<p>tags around all list items (and multipple<p>tags for the multi-line list item). See https://github.com/showdownjs/showdown/wiki/Showdown's-Markdown-syntax#known-differences-and-gotchas.CKEditor, on the other hand, forbids paragraphs inside list items. Both are "block" elements in its schema; see https://ckeditor.com/docs/ckeditor5/latest/framework/guides/deep-dive/schema.html#defining-additional-semantics and block elements cannot contain other block elements. (There is discussion of having a list variant that can contain block elements; see ckeditor/ckeditor5#2973 for more.)
So here we add a Showdown rule to replace list item paragraph tags with linebreaks, the same as CKEditor produces.
How should this be manually tested?
Create a page with list items where the list item contains line breaks (shift+enter). Save the page and re-open. It should look the same as it did before you saved it.
Except it won't look the same if you insert multiple consecutive line breaks. They'll get collapsed into one. That's a separate issue, and not related to list items. And it seems like a less serious issue than #935
Screenshots (if appropriate)