fix: title image rendering in task preview and task view.#58555
fix: title image rendering in task preview and task view.#58555nkuoch merged 11 commits intoExpensify:mainfrom
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…ist. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
… report. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@mananjadhav 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Expensify/design, please confirm if this looks fine or not, specially the image rendering and the gap between link and code block. Monosnap.screencast.2025-03-17.20-17-44.mp4 |
|
Hmm I feel like we don't want to support the image markdown anywhere - not in the title, not in the edit title screen, nor in the system message. It feels weird to show it in one place but not the other. Can we just remove it from all of those cases I just cited? |
Totally agree. |
|
@mananjadhav, do you know how to disable image rendering in Markdown input? I'm really struggling with that. 🥴 |
I'll also have to check it. I think the way it'll need to be done is override the Can you try this and also may be post on expensify-open-source? Meanwhile can we skip the image part in this PR and focus on the ones we can solve? |
|
@mananjadhav thanks, I'll try that. We can leave image rendering issue on inputs. This PR covers the image rendering in task preview, task view title, task confirmation page and task title update system message.
Other linked issues in this PR are solved. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| setTaskReport(report); | ||
| }, [report]); | ||
| const taskTitle = `<task-title>${convertToLTR(report?.reportName ?? '')}</task-title>`; | ||
| const htmlWithoutImage = Parser.replace(Parser.htmlToMarkdown(report?.reportName ?? ''), {disabledRules: ['image']}); |
There was a problem hiding this comment.
htmlWithoutImage is pretty generic name. Can we name it as titleWithoutImage?
| data: [ | ||
| { | ||
| text: `${translate('search.searchIn')} ${reportForContextualSearch.text ?? reportForContextualSearch.alternateText}`, | ||
| text: StringUtils.lineBreaksToSpaces(`${translate('search.searchIn')} ${reportForContextualSearch.text ?? reportForContextualSearch.alternateText}`), |
There was a problem hiding this comment.
To fix this issue where search router was displaying task reports with multiline title. Task - Task with multiline title is not displayed correctly in search list #58192
src/libs/ReportUtils.ts
Outdated
| * For longer comments, skip parsing, but still escape the text, and display plaintext for performance reasons. It takes over 40s to parse a 100k long string!! | ||
| */ | ||
| function getParsedComment(text: string, parsingDetails?: ParsingDetails, mediaAttributes?: Record<string, string>): string { | ||
| function getParsedComment(text: string, parsingDetails?: ParsingDetails, mediaAttributes?: Record<string, string>, isTaskAction?: boolean): string { |
There was a problem hiding this comment.
how about instead we pass disabledRules param?
| } | ||
| if (isInsideTaskTitle) { | ||
| return 19; | ||
| return 18; |
There was a problem hiding this comment.
@Expensify/design can you confirm the value?
| textDecorationStyle: 'solid', | ||
| }, | ||
| underlineLine: { | ||
| textDecorationLine: 'underline', |
There was a problem hiding this comment.
removed, this will be needed in a different PR related to regressions.
mananjadhav
left a comment
There was a problem hiding this comment.
Some clarifications and refactor comments.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const [taskReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`); | ||
| const taskTitle = taskReport?.reportName ?? action?.childReportName ?? ''; | ||
|
|
||
| const taskTitleWithoutImage = Parser.replace(Parser.htmlToMarkdown(taskTitle), {disabledRules: ['image']}); |
There was a problem hiding this comment.
Why not use the CONST here?
| setTaskReport(report); | ||
| }, [report]); | ||
| const taskTitle = `<task-title>${convertToLTR(report?.reportName ?? '')}</task-title>`; | ||
| const titleWithoutImage = Parser.replace(Parser.htmlToMarkdown(report?.reportName ?? ''), {disabledRules: ['image']}); |
There was a problem hiding this comment.
Same use the defined CONST?
| * For longer comments, skip parsing, but still escape the text, and display plaintext for performance reasons. It takes over 40s to parse a 100k long string!! | ||
| */ | ||
| function getParsedComment(text: string, parsingDetails?: ParsingDetails, mediaAttributes?: Record<string, string>): string { | ||
| function getParsedComment(text: string, parsingDetails?: ParsingDetails, mediaAttributes?: Record<string, string>, disabledRules?: string[]): string { |
There was a problem hiding this comment.
Can these by better typed? Say disabledRules?: MarkdownType[] ?
There was a problem hiding this comment.
MarkdownType[] is has different properties, it does not have image.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
I removed the fix for [$50] Chat - Bold markdown is not applied for url #58349, will fix that in this PR. |
|
@mananjadhav, could you please review this last time? I think we are almost ready to merge this one. |
|
I did the review. I need to test this. Will be doing that today. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-task-title-fixes-1.movandroid-task-title-fallback.movAndroid: mWeb Chromemweb-chrome-task-title-fixes-1.moviOS: Nativeios-task-title-fixes-1.moviOS: mWeb Safarimweb-safari-task-title-fixes-1.movMacOS: Chrome / Safariweb-task-title-fixes-1.movMacOS: Desktopdesktop-task-title-fixes-1.mov |
|
@Expensify/design Any comments on these updates |
|
Hmm. I would expect us to not render the image or the markdown text. I'd expect us to only show the link and the linked text, but not the full url and markdown bit if that makes sense. cc @Expensify/design for gut check here |
Can you explain a bit more? I'm not sure I'm following. |
|
Team, while the image rendering is being discussed, I was wondering if we could merge this and images can be a follow up. I've got 2 other PRs blocked by this one that also needs to be merged. What does everyone think? |
|
I'm cool with that personally. |
|
Same here - fine by me! |
|
As long as we do a follow-up that's fine with me as well 👍 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 9.1.20-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.20-2 🚀
|


Explanation of Change
Fixed Issues
$ #58194
$ #58192
$ #58181
$ #58269
PROPOSAL: NA
Tests
Test 2
Test 3
Test 4
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
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))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_native.mp4
Android: mWeb Chrome
android_native.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4