-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: title image rendering in task preview and task view. #58555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: krishna2323 <[email protected]>
…ist. Signed-off-by: krishna2323 <[email protected]>
… report. Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@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 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@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 <[email protected]>
@@ -44,7 +45,8 @@ function TaskView({report}: TaskViewProps) { | |||
useEffect(() => { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
htmlWithoutImage is pretty generic name. Can we name it as titleWithoutImage
?
@@ -145,7 +145,7 @@ function SearchRouter({onRouterClose, shouldHideInputCaret}: SearchRouterProps, | |||
{ | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -4862,7 +4862,7 @@ function completeShortMention(text: string): string { | |||
* For comments shorter than or equal to 10k chars, convert the comment from MD into HTML because that's how it is stored in the database | |||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about instead we pass disabledRules
param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
@@ -635,7 +635,7 @@ function getCodeFontSize(isInsideH1: boolean, isInsideTaskTitle?: boolean) { | |||
return 15; | |||
} | |||
if (isInsideTaskTitle) { | |||
return 19; | |||
return 18; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Expensify/design can you confirm the value?
@@ -9,4 +9,8 @@ export default { | |||
textDecorationLine: 'underline line-through', | |||
textDecorationStyle: 'solid', | |||
}, | |||
underlineLine: { | |||
textDecorationLine: 'underline', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, this will be needed in a different PR related to regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some clarifications and refactor comments.
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@@ -64,6 +65,9 @@ function TaskPreview({taskReportID, action, contextMenuAnchor, chatReportID, che | |||
const {translate} = useLocalize(); | |||
const theme = useTheme(); | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the CONST here?
@@ -44,7 +45,8 @@ function TaskView({report}: TaskViewProps) { | |||
useEffect(() => { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same use the defined CONST?
@@ -4862,7 +4862,7 @@ function completeShortMention(text: string): string { | |||
* For comments shorter than or equal to 10k chars, convert the comment from MD into HTML because that's how it is stored in the database | |||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these by better typed? Say disabledRules?: MarkdownType[]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MarkdownType[]
is has different properties, it does not have image
.
@@ -64,6 +65,9 @@ function TaskPreview({taskReportID, action, contextMenuAnchor, chatReportID, che | |||
const {translate} = useLocalize(); | |||
const theme = useTheme(); | |||
const [taskReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`); | |||
const taskTitle = action?.childReportName ?? taskReport?.reportName ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which change is currently fixing the issue #58269 ?
@mananjadhav, I missed that initially, its now updated. We had to used action?.childReportName
if it's available.
Signed-off-by: krishna2323 <[email protected]>
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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