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-list] Feature: Continuous list numbering #7059

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

patrick-atticus
Copy link
Contributor

Description

Currently, each new numbered ListNode starts it's numbering at "1". The user can create a new list with a custom start value by typing the number, followed by a period, followed by space (eg. "12. "). If the user tries to create continuous lists this way, they will not stay in sync when new items are added/deleted from previous lists.

Continuous lists are a feature of word/google docs accessed via the context menu and are important in certain types of documents:
Screenshot 2025-01-14 at 3 22 13 PM

This PR adds this feature via the following changes:

  • A new property __continuePreviousNumbering to ListNode
  • ListNode method to update the numbering
  • Listeners in ListPlugin to trigger updates

Test plan

Included basic unit and playground tests, can add more if this PR is promising.

Demo:

CleanShot.2025-01-17.at.11.30.15.mp4

Copy link

vercel bot commented Jan 17, 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 17, 2025 0:48am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2025 0:48am

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

size-limit report 📦

Path Size
lexical - cjs 29.03 KB (0%)
lexical - esm 28.87 KB (0%)
@lexical/rich-text - cjs 37.97 KB (0%)
@lexical/rich-text - esm 30.92 KB (0%)
@lexical/plain-text - cjs 36.52 KB (0%)
@lexical/plain-text - esm 28.13 KB (0%)
@lexical/react - cjs 39.79 KB (0%)
@lexical/react - esm 32.28 KB (0%)

@@ -97,10 +97,35 @@ export function registerList(editor: LexicalEditor): () => void {
},
COMMAND_PRIORITY_LOW,
),
editor.registerNodeTransform(ListNode, $updateListNumbering),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to use node transforms where possible, but ran in to a bug with this approach (see video).

I know mutation listeners should not trigger additional updates, but one is needed to handle deleted list events. Would it be preferable to use it for all updates in this case?

CleanShot.2025-01-17.at.11.39.22-converted.mp4

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updating from an update listener is the same anti-pattern as updating from a mutation listener. The reason for an update should always have something to do with something external to the editor's state.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding deletion I don't think there's a good way to react to a deletion that doesn't cause an update cascade, unattached nodes are ignored from transforms, something else would have to be added to the editor API in order to support this, whether that's something like an update listener that runs before transforms or a registerTransform-like API that executes in the cases when nodes are no longer attached.

I think a solution that depends more on DOM manipulation and CSS counters can be achieved without something like this though, shouldn't really even need a transform to support it.

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.

Ideally CSS counters could be leveraged to support this feature and let the browser do most of the work, this implementation approach seems problematic (looks like tests are failing too)

Comment on lines +110 to +112
if (shouldUpdate) {
editor.update($updateListNumbering);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the update cascade anti-pattern. Mutation listeners are intended to be used for updating DOM or sending state out of Lexical but it's not a good idea to trigger another update in here. Code like this is not good for performance and can cause visual artifacts (two reconciliations for each related update) and and infinite updates when there's a bug.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants