-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Completed task does not show strikethrough style. #58629
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]>
Signed-off-by: krishna2323 <[email protected]>
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.
Code changes look fine. Waiting on the #58555 to be merged.
@Krishna2323 Is this PR ready? Can this be merged while the second one is in the review? |
@mananjadhav, I think it's better to merge this PR before merging this one to avoid regressions. |
@Krishna2323 Can we now resolve conflicts and open the PR? The dependent PR is merged. |
Signed-off-by: krishna2323 <[email protected]>
@@ -89,6 +89,14 @@ function TaskPreview({taskReportID, action, contextMenuAnchor, chatReportID, che | |||
return <RenderHTML html={`<deleted-action>${translate('parentReportAction.deletedTask')}</deleted-action>`} />; | |||
} | |||
|
|||
const getTaskHTML = () => { |
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.
does it make sense to use useMemo?
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.
I don't see any value in adding useMemo here. I think we can use useMemo on line 70. WDYT?
const taskTitleWithoutImage = Parser.replace(Parser.htmlToMarkdown(taskTitle), {disabledRules: [...CONST.TASK_TITLE_DISABLED_RULES]});
@mananjadhav, the build is failing for the native apps. Do you know anything that might help? Thanks. ![]() ![]() |
@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] |
I haven't come across the second error. The first one is generally if you don't have the RN script running. Can you try |
@Krishna2323 is this ready? The native apps are now need a separate |
@mananjadhav, thanks for the help🙇🏻. This is now ready. |
Reviewer Checklist
Screenshots/Videos |
✋ 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/MariaHCD in version: 9.1.27-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.28-15 🚀
|
Explanation of Change
Fixed Issues
$ #58265
PROPOSAL: NA
Tests
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_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4