-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Task - Mark down is not copied when task preview is copied to clipboard. #58572
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
fix: Task - Mark down is not copied when task preview is copied to clipboard. #58572
Conversation
…ipboard. Signed-off-by: krishna2323 <[email protected]>
…tle field in task report. Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
…s used in task title. Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
src/components/HTMLEngineProvider/HTMLRenderers/BlockQuoteRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/StrongRenderer.tsx
Outdated
Show resolved
Hide resolved
/** | ||
* Remove pre tag from the html | ||
*/ | ||
function removePreCodeBlock(text = '') { |
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.
do we have tests for this method
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.
The function is quite simple, and a test might not add much value.
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 we add tests for the methods?
@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]>
@Krishna2323 Can you merge the latest main and resolve conflicts? |
Signed-off-by: krishna2323 <[email protected]>
@Krishna2323 The screenshots are missing for all the platforms. Can you please go through the checklist once more and ensure you've only checked off after verifying each item? |
Signed-off-by: krishna2323 <[email protected]>
} | ||
return styles.onlyEmojisTextLineHeight; | ||
return HTMLEngineUtils.isChildOfTaskTitle(tnode as TNode) ? {} : {...styles.blockquote, ...styles.onlyEmojisTextLineHeight}; |
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.
@mananjadhav, I removed the custom blockquote renderer file after learning a better way to override styles for the task title from @bernhardoj's PR. This approach works really well for our case.
@mananjadhav, I have added the screenshots. |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Testing this PR. |
I think this looks fine to me, but just a cc to @Expensify/design that we keep running into these weird edge cases for markdown support now. |
Totally agree. I think at some point it would be nice to come up with a really tight, global strategy on how we handle markdown throughout the app. It's getting a bit wild. |
@Krishna2323 Can you please resolve the conflicts? |
@mananjadhav, done. |
Yeah, that seems fine to me 👍 |
Thanks for confirming. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-block-quote-task.movAndroid: mWeb Chromemweb-chrome-block-quote-task.moviOS: Nativeios-block-quote-task.moviOS: mWeb Safarimweb-safari-block-quote-task.movMacOS: Chrome / Safariweb-block-quote-task.movMacOS: Desktopdesktop-block-quote-task.mov |
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.
Looks good, thank you!
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.1.23-1 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.23-7 🚀
|
Explanation of Change
Fixed Issues
$ #58154
$ #58137
$ #58136
PROPOSAL: NA
Tests
Test 2
Test 3
> hey
and save it.hey
is rendered as normal task title text.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