[lexical-list] Feature: Preserve ordered list numbering when split by blocks or paragraphs#8092
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
etrepum
left a comment
There was a problem hiding this comment.
Overall this looks like a good direction, but it can be done in less code, and there should be new tests to show that this new behavior works as expected.
| const listType = parent.getListType(); | ||
| let newStart = 1; | ||
|
|
||
| if (listType === 'number') { | ||
| newStart = parent.getStart() + listItem.getIndexWithinParent(); | ||
| } | ||
|
|
||
| const newList = $createListNode(parent.getListType(), newStart); |
There was a problem hiding this comment.
Since this code is basically repeated 3 times maybe it should be consolidated into a function
| let nextSibling = this.getNextSibling(); | ||
| const nextSiblings = []; | ||
| while (nextSibling) { | ||
| const nodeToAppend = nextSibling; | ||
| nextSiblings.push(nextSibling); | ||
| nextSibling = nextSibling.getNextSibling(); | ||
| newList.append(nodeToAppend); | ||
| } |
There was a problem hiding this comment.
This is a long way of writing
const nextSiblings = this.getNextSiblings();There was a problem hiding this comment.
Thanks for the guidance! Its a real honor to learn from a mentor like you.
I've refactored the code to use getNextSiblings() and consolidated the start index calculation into a $getNewListStart helper in utils.ts to reduce duplication. I also added a regression test in LexicalListItemNode.test.ts to verify numbering continuity when splitting lists.
|
Hello, @Sa-Te. If this becomes the default behavior, how can I reset the counter for the split list section? I'd also like to be able to configure this as an option. I think that for some users this change will come as a surprise and will require additional redesign of those services that are integrated with lexical-based editors |
Cheers for the feedback, mate! As for resetting the counter manually, that would likely be handled by the UI (like a 'Restart Numbering' context menu) manipulating the 'start' attribute, rather than relying on the default split behaviour. I’m happy to leave it to the maintainers to decide if this needs to be behind a configuration flag, but personally, I reckon bringing it in line with standard rich-text expectations is the way forward. |
Google Docs and Word really don't reset the counter, and I understand the motivation to bring the behavior in line with popular editors. But this is only one of the ways a list can be split. For example, Notion, just like lexical in its current state, splits list into separate nodes when you try to insert a paragraph between items. I'm not opposed to your change, but if you made it possible to reset the new default behavior, it would make the feature truly excellent |
etrepum
left a comment
There was a problem hiding this comment.
What do you think about adding an option to the list extension that defaults to the previous behavior, but can be configured to have this new behavior? I think that would solve the compatibility issue with people that want to preserve the existing behavior
Thanks @etrepum and @levensta for the feedback! I've refactored the approach to be fully opt-in and backward compatible, moving the logic from the Node level to the Command level. Changes in this update: Configurable Logic: Updated $handleListInsertParagraph (and registerList) to accept a shouldPreserveNumbering option. React Plugin: Updated to accept a shouldPreserveNumbering prop. Playground: Enabled this feature by default in the Playground so the "smart" behavior is visible there. Tests: Added unit tests covering both scenarios: Note: I have disabled the feature in the Playground Editor.tsx by default to ensure existing E2E tests pass. The logic is verified via Unit Tests. |
etrepum
left a comment
There was a problem hiding this comment.
I'll give this another review when the build passes, there seem to be a lot of new errors
3715316 to
5e9cc8d
Compare
etrepum
left a comment
There was a problem hiding this comment.
This is missing support in ListExtension, the configuration should also be in its ListConfig.
packages/lexical-list/src/index.ts
Outdated
| export type RegisterListOptions = { | ||
| restoreNumbering?: boolean; | ||
| }; |
There was a problem hiding this comment.
| export type RegisterListOptions = { | |
| restoreNumbering?: boolean; | |
| }; | |
| export interface RegisterListOptions { | |
| restoreNumbering?: boolean; | |
| } |
packages/lexical-list/src/utils.ts
Outdated
| const children = list.getChildren(); | ||
| const index = children.indexOf(listItem); |
There was a problem hiding this comment.
| const children = list.getChildren(); | |
| const index = children.indexOf(listItem); | |
| const index = listItem.getIndexWithinParent(); |
…agraph and also fixed errors due to version mismatches
5e9cc8d to
52343d4
Compare
… blocks or paragraphs (facebook#8092)
Summary
Fixes #8037
Currently, when an Ordered List is interrupted by a Block Node (like a Code Block) or split via the Enter key (creating a Paragraph), the subsequent list resets its numbering to
1.This PR fixes the numbering continuity by:
LexicalListItemNode&formatList): Changed the list splitting logic to calculate thenewStartindex based on the splitting item'sindexWithinParentrather than relying on fragile sibling counts.LexicalListNode): Added a check inupdateDOMto ensure changes to thestartattribute are actually applied to the DOM.Test Plan
repro steps:
Expected Behavior (Fixed):
The list below the block should continue numbering at 3.
Actual Behavior (Before Fix):
The list below the block resets to 1.
Implementation Details
prevNode.__start !== this.__startcheck inupdateDOM.replaceandinsertAfterto calculatenewStartusinggetIndexWithinParent().$handleListInsertParagraphto calculatenewStartusinggetIndexWithinParent()when splitting a list with a paragraph.