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.

Sign up for GitHub

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

Jump to bottom

Add indented list support #1658

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Add indented list support #1658

merged 1 commit into from
Apr 13, 2022

Conversation

fantactuka
Copy link
Contributor

  • Indented lists
  • Merge lists
  • Inline code

@fantactuka fantactuka requested a review from kraisler April 12, 2022 14:20
@vercel
Copy link

vercel bot commented Apr 12, 2022 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

lexical – ./packages/lexical-website

🔍 Inspect: https://vercel.com/fbopensource/lexical/GsBeCtq2J8ewda8hAx6o1hk1HjSo
✅ Preview: https://lexical-git-markdown-indented-list-fbopensource.vercel.app

lexical-playground – ./packages/lexical-playground

🔍 Inspect: https://vercel.com/fbopensource/lexical-playground/5rPnsjLjAoQfxbTDXahxXCnKkbCk
✅ Preview: https://lexical-playground-git-markdown-indented-list-fbopensource.vercel.app

lexical-website-new – ./packages/lexical-website-new

🔍 Inspect: https://vercel.com/fbopensource/lexical-website-new/FigBgiGaLbSoiddEcEaV6j73K9Ky
✅ Preview: https://lexical-website-new-git-markdown-indented-list-fbopensource.vercel.app

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 12, 2022
acywatson
acywatson reviewed Apr 12, 2022
View reviewed changes
Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

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

Seems fine, but should we add a test or two?

@fantactuka fantactuka changed the base branch from markdownForPC1 to main April 12, 2022 15:49
@fantactuka fantactuka force-pushed the markdown-indented-list branch from d044313 to 47365fd Compare April 12, 2022 16:52
@fantactuka fantactuka force-pushed the markdown-indented-list branch from 47365fd to 699cca5 Compare April 12, 2022 16:53
@fantactuka fantactuka force-pushed the markdown-indented-list branch from 699cca5 to 04739a2 Compare April 12, 2022 16:54
acywatson
acywatson reviewed Apr 12, 2022
View reviewed changes
packages/lexical-markdown/src/utils.js
@@ -212,15 +214,15 @@ const markdownBlockQuote: MarkdownCriteria = {
const markdownUnorderedListDash: MarkdownCriteria = {
...paragraphStartBase,
markdownFormatKind: 'paragraphUnorderedList',
regEx: /^(?:- )/,
regExForAutoFormatting: /^(?:- )/,
regEx: /^(\s{0,10})(?:- )/,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this 10 be a configurable constant? This is max indent level, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's actually no hard requirement to have limit here at all, IIRC we have 10 as max indention level anyways so even if user puts 30 spaces it still 10 nested lists max. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea it makes sense...someone made a MaxIndentLevel plugin though, I think, so I wonder how this interacts with that if you set the limit to like 5 in the plugin props.

kraisler
kraisler reviewed Apr 12, 2022
View reviewed changes
Copy link
Contributor

@kraisler kraisler left a comment

Choose a reason for hiding this comment

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

Great PR. I'll wait until you address Acy's feedback before approving.

packages/lexical-markdown/src/utils.js Outdated
const listItem = $createListItemNode();
const indentMatch =
patternMatchResults.regExCaptureGroups[0].text.match(/^\s*/);
const indent = indentMatch ? Math.floor(indentMatch[0].length / 2) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use 4 vs 2 to match existing PC behavior.

kraisler
kraisler approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@kraisler kraisler left a comment

Choose a reason for hiding this comment

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

Looks like Acy's comments were not blockers.

@fantactuka fantactuka force-pushed the markdown-indented-list branch from 04739a2 to 08a6e4f Compare April 12, 2022 20:24
@fantactuka fantactuka force-pushed the markdown-indented-list branch from 08a6e4f to bc6e080 Compare April 12, 2022 20:34
@fantactuka fantactuka merged commit 8f45503 into main Apr 13, 2022
@fantactuka fantactuka deleted the markdown-indented-list branch April 13, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acywatson acywatson acywatson left review comments

@kraisler kraisler kraisler approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fantactuka @kraisler @acywatson @facebook-github-bot