Add OL renderer#90482
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9732fc7a3d
ℹ️ 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".
| let counter = 0; | ||
| for (const [i, child] of children.entries()) { | ||
| if (child.tagName === 'li') { | ||
| counter += 1; | ||
| map.set(i, counter); |
There was a problem hiding this comment.
- numbering attributes when rendering markers
The new ordered-list renderer always starts numbering from 1 and increments per <li> (counter = 0 then counter += 1), so it ignores standard list attributes like <ol start>, <ol reversed>, <ol type>, and per-item value. In any HTML payload that uses those attributes (for example, continuation lists starting at 3), the rendered numbering becomes incorrect (1., 2., ...), which changes the meaning of ordered instructions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Question weather we need it? Do we expect concierge to use those ?
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@roryabraham Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9732fc7a3d
ℹ️ 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".
| let counter = 0; | ||
| for (const [i, child] of children.entries()) { | ||
| if (child.tagName === 'li') { | ||
| counter += 1; | ||
| map.set(i, counter); |
There was a problem hiding this comment.
Preserve ordered-list numbering attributes
The custom <ol> renderer always derives marker numbers from a local counter (0 -> 1, 2, ...) and never reads list/item attributes, so ordered lists with start, reversed, type, or <li value> now render incorrect numbering (e.g., continuation steps restart at 1.). This is a functional regression from native ordered-list semantics and can change instruction meaning in concierge responses or imported HTML. Fresh evidence in this commit: buildLiIndices() only increments counter, and the new tests assert only 1./2. paths without any attribute-based cases.
Useful? React with 👍 / 👎.
Explanation of Change
This PR mirrors #89162 changes for
OL, so:<br/>rendered as an extra empty list item at the end of the ordered list.<ul>renderer with a custom one, fixing the spacing.Fixed Issues
$ #89039
PROPOSAL:
Tests
Optionally for
2you can paste:into console on a chat / report view.
Offline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
iOS: Native
MacOS: Chrome / Safari