fix(rich-text-editor): allow any block inside list item #157

KyleMit · 2022-07-07T13:55:51Z

Closes #63

Describe your changes

Schemas dictate which elements are allowed to appear under each node. The previous schema for list item content was paragraph block*, meaning the first child must be a paragraph then 0 or more blocks under that.

If we want to allow blockquotes directly inside a list item, we'll have to open up and just allow block+, meaning one or more of any of the block level elements that we've allowed - including paragraphs, as well as blockquotes, codeblocks, etc.

PR Checklist

All new/changed functionality includes unit and (optionally) e2e tests as appropriate
All new/changed functions have /** ... */ docs (no changes to top level functions)
I've added the bug/enhancement and other labels as appropriate

Additional context

Change is a somewhat simple one here. One thing I wanted to investigate in making this change is how other rich text editors handle this in the UI / Action.

Jira - Will exit list, create new-line, and start blockquote there
Reddit - Will exit list, and convert list item to blockquote
Default ProseMirror Editor - does not allow, because default schema only allows paragraph items as the direct child of a list item

So support for this specific feature elsewhere is not widespread. We already do allow someone to create a new line within a list item and insert a blockquote on it's own line.

That said, if our goal is to ever have parity between markdown editor and rich text editor, any content restrictions will hurt our ability to do that. In markdown, you are certainly allowed to type * > blockquote list item - so anyone starting from markdown can get themselves into a situation that our Rich Text schema would consider "invalid" if we have strict schema policies. So it's probably better to relax them, even if it leads to some odd (but perfectly valid) markup

netlify · 2022-07-07T13:56:48Z

✅ Deploy Preview for nifty-lalande-39c157 ready!

Name	Link
🔨 Latest commit	`49acd27`
🔍 Latest deploy log	https://app.netlify.com/sites/nifty-lalande-39c157/deploys/62d880bdcd0511000829d8a2
😎 Deploy Preview	https://deploy-preview-157--nifty-lalande-39c157.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

KyleMit · 2022-07-11T19:16:59Z

src/rich-text/schema.ts

@@ -169,7 +169,7 @@ const nodes: {
    },

    list_item: {
-        content: "paragraph block*",
+        content: "block+",


The only change to allow blockquotes in list items, is from forcing the content to have the first child be a paragraph with 0 or more block decadents, to having one or more of any block level element

Just double checked the commonmark spec for absolute correctness™, and it seems that list items allow basically any block element that isn't a horizontal rule, so this is good to go.

https://spec.commonmark.org/0.30/#list-items

KyleMit · 2022-07-11T19:18:28Z

src/rich-text/commands/index.ts

@@ -37,7 +37,7 @@ import { _t } from "../../shared/localization";
 export * from "./tables";

 //TODO
-function toggleWrapIn(nodeType: NodeType) {
+export function toggleWrapIn(nodeType: NodeType) {


needed to test blockquote command which is generated via this method. Other commands (like toggleHeadingLevel) are exported from here as well, so no over-arching reason to keep private

If you expose this method, you need to write documentation and unit tests for it 😉

so, in a sense, this is actually the system under test when testing blockquotes can be inserted into lists. What we want to check is a) can we execute this command and b) does it apply the right transform. So there's a single existing test for it, but I added some more test cases for some basic coverage of toggling back and forth

b-kelly

This change seems appropriate to me. You'll need to add new tests and documentation for the newly exported function though.

b-kelly · 2022-07-13T18:57:44Z

src/rich-text/commands/index.ts

@@ -37,7 +37,7 @@ import { _t } from "../../shared/localization";
 export * from "./tables";

 //TODO
-function toggleWrapIn(nodeType: NodeType) {
+export function toggleWrapIn(nodeType: NodeType) {


If you expose this method, you need to write documentation and unit tests for it 😉

KyleMit · 2022-07-18T18:48:40Z

@b-kelly - should be all set - added node tree string to custom matchers

b-kelly

I'd like to add some parsing/serialization tests here as well. I'll do this later today before merge. Otherwise, I think this all looks good

b-kelly · 2022-07-20T20:10:37Z

@KyleMit I've added some markdown parsing and serializing tests - can you look into the failing one for me?

KyleMit · 2022-07-20T22:32:18Z

@b-kelly, biggest gap was that there was a <code> block, but not the actual text inside it. Also some white space stuff - valid markup to just adding four spaces to create a code block, even in a nested list - and that's what the markdown serializer is doing, so adding that into the test. Should be fixed now

b-kelly · 2022-07-25T14:34:44Z

@KyleMit 🤦 Wow, that was a dumb mistake on my part! I knew the test should be working, but I guess I was too braindead at the time to realize it was that simple. I'll give this one last pass today and merge.

allow any block inside list item

b1892bc

KyleMit changed the title ~~allow any block inside list item~~ fix: allow any block inside list item Jul 7, 2022

add test coverage for inserting blockquote into list item

d6f002a

KyleMit added the bug Something isn't working label Jul 11, 2022

KyleMit requested a review from a team July 11, 2022 19:12

linting

3392680

KyleMit marked this pull request as ready for review July 11, 2022 19:14

KyleMit changed the title ~~fix: allow any block inside list item~~ fix(rich-text-editor): allow any block inside list item Jul 11, 2022

KyleMit commented Jul 11, 2022

View reviewed changes

KyleMit requested a review from b-kelly July 12, 2022 11:39

b-kelly requested changes Jul 13, 2022

View reviewed changes

KyleMit added 2 commits July 14, 2022 13:10

refactor(tests): add createBasicNodeTree test helper

44e645a

test(rich-text-editor) add toggleWrapIn tests

12360c1

KyleMit requested a review from b-kelly July 14, 2022 17:15

KyleMit added 5 commits July 14, 2022 13:22

chore(tests): remove unused test method

feae786

docs(commands): improved toggleWrapIn jsdoc comment

526f3c8

docs(matchers): add doc comment for createBasicNodeTree

63f7965

move toMatchNodeTreeString to custom matchers

acffe45

fix(formatting): prettier --write

4b6d813

b-kelly reviewed Jul 19, 2022

View reviewed changes

Merge branch 'main' into kylemit/blocks-inside-list-item

dc6cf5a

b-kelly added a commit that referenced this pull request Jul 20, 2022

test(matchers): add toMatchNodeTreeString matcher from #157

4ad3c6d

b-kelly added 5 commits July 20, 2022 15:17

Merge branch 'main' into kylemit/blocks-inside-list-item

7d71e50

drop file that tests tests

d3e769c

Merge branch 'main' into kylemit/blocks-inside-list-item

3328b2b

add markdown-parser tests

97eeabd

add markdown-serializer tests

6878f95

fix breaking test

49acd27

KyleMit requested a review from b-kelly July 21, 2022 15:57

b-kelly merged commit f59b72f into main Jul 25, 2022

b-kelly deleted the kylemit/blocks-inside-list-item branch July 25, 2022 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rich-text-editor): allow any block inside list item #157

fix(rich-text-editor): allow any block inside list item #157

KyleMit commented Jul 7, 2022 •

edited

Loading

netlify bot commented Jul 7, 2022 •

edited

Loading

KyleMit Jul 11, 2022

b-kelly Jul 20, 2022 •

edited

Loading

KyleMit Jul 11, 2022

b-kelly Jul 13, 2022

KyleMit Jul 14, 2022

b-kelly left a comment

b-kelly Jul 13, 2022

KyleMit commented Jul 18, 2022

b-kelly left a comment

b-kelly commented Jul 20, 2022

KyleMit commented Jul 20, 2022

b-kelly commented Jul 25, 2022

fix(rich-text-editor): allow any block inside list item #157

fix(rich-text-editor): allow any block inside list item #157

Conversation

KyleMit commented Jul 7, 2022 • edited Loading

Describe your changes

PR Checklist

Additional context

netlify bot commented Jul 7, 2022 • edited Loading

✅ Deploy Preview for nifty-lalande-39c157 ready!

KyleMit Jul 11, 2022

Choose a reason for hiding this comment

b-kelly Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

KyleMit Jul 11, 2022

Choose a reason for hiding this comment

b-kelly Jul 13, 2022

Choose a reason for hiding this comment

KyleMit Jul 14, 2022

Choose a reason for hiding this comment

b-kelly left a comment

Choose a reason for hiding this comment

b-kelly Jul 13, 2022

Choose a reason for hiding this comment

KyleMit commented Jul 18, 2022

b-kelly left a comment

Choose a reason for hiding this comment

b-kelly commented Jul 20, 2022

KyleMit commented Jul 20, 2022

b-kelly commented Jul 25, 2022

KyleMit commented Jul 7, 2022 •

edited

Loading

netlify bot commented Jul 7, 2022 •

edited

Loading

b-kelly Jul 20, 2022 •

edited

Loading