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

[lexical-react] Feature: Merge TabIndentionPlugin and ListMaxIndentLevelPlugin plugins #7018

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

fantactuka
Copy link
Contributor

@fantactuka fantactuka commented Jan 6, 2025

Moved max indent logic to TabIndentionPlugin and updated it to work on non-list elements too

Copy link

vercel bot commented Jan 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 4:15pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 4:15pm

@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 Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

size-limit report 📦

Path Size
lexical - cjs 31.39 KB (0%)
lexical - esm 31.19 KB (0%)
@lexical/rich-text - cjs 40.47 KB (0%)
@lexical/rich-text - esm 33.12 KB (0%)
@lexical/plain-text - cjs 39.04 KB (0%)
@lexical/plain-text - esm 30.37 KB (0%)
@lexical/react - cjs 42.3 KB (0%)
@lexical/react - esm 34.42 KB (0%)

@fantactuka
Copy link
Contributor Author

Tables.spec.mjs:4078:3 › Tables › Paste and insert new lines after unmerging cells 

this one might be flaky, will re-run failures

@etrepum etrepum changed the title Merge TabIndentionPlugin and ListMaxIndentLevelPlugin plugins [Breaking Change][lexical-react] Feature: Merge TabIndentionPlugin and ListMaxIndentLevelPlugin plugins Jan 6, 2025
@fantactuka
Copy link
Contributor Author

@etrepum we didn't expose ListMaxIndentLevelPlugin from playground and new maxIndent prop is optional, shouldn't be a breaking change or am I missing something

@etrepum
Copy link
Collaborator

etrepum commented Jan 6, 2025

Sorry, you're right, I didn't look carefully when I was making that change. Don't have time for a full review right now, but was doing a quick fix up according to the template

@etrepum etrepum changed the title [Breaking Change][lexical-react] Feature: Merge TabIndentionPlugin and ListMaxIndentLevelPlugin plugins [lexical-react] Feature: Merge TabIndentionPlugin and ListMaxIndentLevelPlugin plugins Jan 6, 2025
@acywatson
Copy link
Contributor

One possible issue - it appears we're now coupling these two functionalities. So you can't set a max indent without also enabled tab indents. Usually this probably won't matter, but I did want to point out the change.

@etrepum
Copy link
Collaborator

etrepum commented Jan 7, 2025

I think there might be some subtle issue with collab here? I haven't been able to get a fully clean test run yet, the last two had this failure in common:

[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Selection.spec.mjs:1092:3 › Selection › shift+arrowdown into a table, when the table is the only node, selects the whole table

Currently it's on attempt 13 https://github.com/facebook/lexical/actions/runs/12636094981/job/35283390649?pr=7018

@etrepum
Copy link
Collaborator

etrepum commented Jan 8, 2025

I guess it's just unlucky, this time it had a different failure

[1]   1 failed
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Links.spec.mjs:1969:3 › Links › Can handle pressing Enter inside a Link containing multiple TextNodes 
[1]   11 flaky
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/ListsCopyAndPaste.spec.mjs:115:3 › Lists CopyAndPaste › Copy and paste of partial list items into the list 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/HorizontalRule.spec.mjs:31:3 › HorizontalRule › Can create a horizontal rule and move selection around it 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Links.spec.mjs:431:3 › Links › Can create a link with some text after, insert paragraph, then backspace, it should merge correctly 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Links.spec.mjs:2012:3 › Links › Can handle pressing Enter at the beginning of a Link 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/List.spec.mjs:77:3 › Nested List › Can create a list and partially copy some content out of it 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Selection.spec.mjs:520:3 › Selection › Can delete sibling elements forward 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Selection.spec.mjs:1046:3 › Selection › shift+arrowdown into a table, when the table is the last node, selects the whole table 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Selection.spec.mjs:1069:3 › Selection › shift+arrowup into a table, when the table is the first node, selects the whole table 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Selection.spec.mjs:1092:3 › Selection › shift+arrowdown into a table, when the table is the only node, selects the whole table 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Tables.spec.mjs:1662:3 › Tables › Table selection: can select multiple cells and insert an image 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:42:3 › Toolbar › Insert image caption + table 

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

The logic looks fine, per @acywatson's point I think if someone really wanted to do max indent without tab they could just resurrect the old plug-in or intercept KEY_TAB_COMMAND

@fantactuka
Copy link
Contributor Author

I think this could've been a separate plugin, but then I revisited popular editors and didn't see any of it using indent support without tab key (indent via UI controls only). I'll go ahead with merged version, and let's see if there's any feedback to have it as a standalone one

@fantactuka fantactuka added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 33e3677 Jan 8, 2025
42 of 43 checks passed
@etrepum etrepum mentioned this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants