POC adding new ul and li renderers#89162
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@roryabraham PR is ready for review! However, I'm not able to update the description, so I posted a comment. Could you please paste it? 🙏 Also, I can't mark the PR as opened. Could you also do that, please? |
|
@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] |
|
done @war-in. Looks like ESLint is failing though |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d46cead464
ℹ️ 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".
| a: AnchorRenderer, | ||
| code: CodeRenderer, | ||
| img: ImageRenderer, | ||
| li: BulletItemRenderer, |
There was a problem hiding this comment.
Preserve ordered-list numbering
Registering BulletItemRenderer for every standard li means it also runs for ordered lists, because this renderer always emits CONST.DOT_SEPARATOR. In any HTML passed to RenderHTMLSource like <ol><li>First</li><li>Second</li></ol>, the built-in ordered-list marker is bypassed and the items render as bullets instead of 1., 2., which regresses standard HTML list rendering while adding <ul> support.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's very useful. I'm fixing this now
| */ | ||
| function ULRenderer({tnode, style}: CustomRendererProps<TBlock>) { | ||
| return ( | ||
| <View style={[style, {gap: 8}]}> |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The magic number 8 is used for gap styling without a named constant or comment explaining the design intent. This makes it harder to maintain consistent spacing across the codebase.
Extract this value into a named constant or use an existing spacing variable from the theme/variables system:
// e.g., in CONST or variables
const BULLET_LIST_ITEM_GAP = 8;
// Then in the component:
<View style={[style, {gap: BULLET_LIST_ITEM_GAP}]}>Reviewed at: d46cead | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ul: HTMLElementModel.fromCustomModel({ | ||
| tagName: 'ul', | ||
| contentModel: HTMLContentModel.block, | ||
| mixedUAStyles: {marginVertical: 12}, |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The magic number 12 is used for marginVertical styling without a named constant or comment explaining the design intent.
Extract this value into a named constant or use an existing spacing variable from the theme/variables system:
// e.g., in CONST or variables
const UL_MARGIN_VERTICAL = 12;
// Then in the model definition:
mixedUAStyles: {marginVertical: UL_MARGIN_VERTICAL},Reviewed at: d46cead | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / SafariNote to myself: |
|
The
In short: |
On it, I'll try to investigate the issue 🫡 |
|
@aimane-chnaif mobile platforms should be working now, sorry for the oversight |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Code ReviewOverall this is a well-structured fix. The approach of overriding the library's default A few observations: 1. Magic number in
2. The 3. Consistency note:
Looks good overall — the regex stripping correctly targets only orphaned |
Regression AnalysisNo high-risk regressions found. One minor edge case worth noting. Areas investigated
Edge case: consecutive
|
|
@war-in can we apply same fix to ordered list?
Use this for example data: Btw not blocker sine this is happening on production. I am fine to fix as follow-up, limiting the scope of PR to |
|
@roryabraham looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
tests were passing? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.69-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a technical rendering fix for how |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.69-18 🚀
|







Explanation of Change
Concierge sends HTML bullet lists using standard
<ul>/<li>tags, sometimes with an orphaned<br/>inside the<ul>(e.g.<ul><li>One</li><li>Two</li><br/></ul>). Two problems resulted:<br/>rendered as an extra empty bullet at the end of the list.ulrenderer wraps each child inMarkedListItem, which doesn't match how our existing<bullet-list>/<bullet-item>markup is rendered, and the spacing felt off.This PR fixes both:
HTMLElementModels forulandliinBaseHTMLEngineProvider(mirroring the existingbullet-list/bullet-itemmodels), and appliesmarginVertical: 12to<ul>for consistent spacing.ULRendererthat renders<ul>as a plain block container (a<View>wrappingTNodeChildrenRenderer), bypassing the library's defaultMarkedListItemwrapping. The bullet markers are drawn by the<li>renderer.BulletItemRendererfor<li>, so standard<li>and the legacy<bullet-item>render identically.RenderHTML, strips orphaned<br/>tags that are direct children of<ul>(both the<br/>immediately before</ul>and any<br/>between</li>and the next<li>) before handing the HTML toreact-native-render-html. This prevents the extra empty bullet without touching<br/>usage outside of lists.Fixed Issues
$ #89039
PROPOSAL:
Tests
•).Offline tests
N/A
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari