feat: Add numberingFormat to orderedList#1527
feat: Add numberingFormat to orderedList#1527johanneswilm wants to merge 8 commits intosuperdoc-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…d-1008-list-definitions-support-for-editing-and-creating-new
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
harbournick
force-pushed
the
main
branch
2 times, most recently
from
84f8ea5 to
2a443d8
Compare
…d-1008-list-definitions-support-for-editing-and-creating-new
johanneswilm
deleted the
sd-1008-list-definitions-support-for-editing-and-creating-new
branch
johanneswilm
restored the
sd-1008-list-definitions-support-for-editing-and-creating-new
branch
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@johanneswilm nice work on the collapsed-cursor path — findAdjacentListItems is clean.
one thing to fix: when the selection spans multiple lists, changeListNumberingType merges them into one by assigning a single numId to all paragraphs. grouping by original numId (like the collapsed path does) would keep them separate.
a couple small cleanup suggestions inline. test-wise, changeListNumberingType has no tests yet and the converter tests copy the namespace logic instead of calling the real export — worth addressing.
left inline comments.
| }); | ||
| } | ||
|
|
||
| if (paragraphsInSelection.length === 0) { |
There was a problem hiding this comment.
if the selection spans two different lists, this gives them all the same new numId and merges them into one sequence. the collapsed path already handles this correctly by grouping per numId — the ranged path should do the same.
| * @param {string} numberingType - The numbering format key | ||
| * @returns {string} The SVG icon for the format | ||
| */ | ||
| export function getNumberingIcon(numberingType) { |
There was a problem hiding this comment.
this rebuilds a map with the same keys as numberingIcons. could just be numberingIcons[type] || numberingIcons.decimal.
| }, | ||
| }); | ||
|
|
||
| const setNumberedListIcon = (button, numberingType) => { |
There was a problem hiding this comment.
same logic as superToolbar.updateNumberedListIcon — and superToolbar is already in scope here. one copy is enough.
| ListHelpers.generateNewListDefinition({ numId: Number(numId), listType, editor }); | ||
|
|
||
| // Use the provided numbering format or default settings | ||
| if (listType === 'orderedList' && numberingFormat) { |
There was a problem hiding this comment.
the inner else and the outer else do the same thing. compute formatConfig upfront, then branch once.
|
closing in favor of #2366. |
No description provided.