-
Notifications
You must be signed in to change notification settings - Fork 135
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: unwanted linebreaks added when translating html blockquote to markdown #554
fix: unwanted linebreaks added when translating html blockquote to markdown #554
Conversation
When preparing the PR, I performed testing by checking quote surrounded by text, inline and block elements. I found that currently we have 3 types of block elements( During the testing
case-4a.movcase-4b.mov |
@@ -654,9 +656,6 @@ export default class ExpensiMark { | |||
if (textToFormat !== '') { | |||
replacedText += this.formatTextForQuote(regex, textToFormat, replacement); | |||
} | |||
|
|||
// Replace all the blank lines between quotes, if there are only blank lines between them | |||
replacedText = replacedText.replace(/(<\/blockquote>((\s*)+)<blockquote>)/g, '</blockquote><blockquote>'); |
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.
It's safe to remove this as it's not covered by any unit test.
I think we should retain line breaks between quotes because the App prefers to display user's input as it is.
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.
hmmm can you look into when it was added and why please?
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.
@iwiznia The change was added in this PR #385 which is quite old. This change was included at that time but no unit test case was added together in PR #385. From the corresponding original issue Expensify/App#2670, I found that it fixed retaining only one empty line within a quote if there’re more than one. But later we wanted to retain all empty rows inside a quote, see slack discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1682331970891359.
I have fixed another issue #531 to avoid removing line break before and after heading. The App has been reported many issues about line break handling. And I found that we prefer to keep line breaks from user's input as it is instead of removing them, which is different from markdown rendering on Github.
So, I think, in this case, we can also remove it to keep line breaks between quotes. Strictly speaking, it can be out of the scope of this issue but given it's annoying when performing tests and is easy to fix without breaking existing tests, I prefer to fix it and add tests to cover it together in this PR.
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.
Friendly bump @iwiznia
@iwiznia @narefyev91 I'm happy to hear your thoughts on this to move this issue forward. |
@eh2077 can you please following standard template to contributor PR. Also if issue already exists in production and not related to current one - we can skip it here. The main idea to fix current issue and not produce any regressions |
Ionatan could you please also review as I think you have more context on the issue |
Got it. I'm going to test case 1, 2 and 3 on all platforms and case 4A and 4B on web only. |
@narefyev91 Updated PR check list |
Bump @narefyev91 |
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.
LGTM!
@MonilBhavsar Can you help to merge this PR if you're also happy with the change? cc @iwiznia |
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.
Thanks for adding detailed testing steps!
Fixed Issues
$ Expensify/App#20725
Tests
Open the comment in editing mode and verify that the initial draft is same as input.
Select the comment, copy the selection by Ctr/Cmd + C, paste it to composer and verify that it's same as input
case-1.mov
Add a comment for case 2
Repeat step 2 and 3
case-2.mov
Add a comment for case 3
Repeat step 2 and 3
case-3.mov
Open w3schools, add following html and run
case-4a-w3c.mov
case-4b-w3c.mov
QA
3a. On Web and Desktop, select the comment, copy the selection by Ctr/Cmd + C, paste it to composer and verify that it's same as input
3b. On mobile, long press comment, copy to clipboard, pasting in composer and verify that it's same as input
case-4a-w3c.mov
case-4b-w3c.mov
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
web.mov
case-4a-w3c.mov
case-4b-w3c.mov
Mobile Web - Chrome
mobile-chrome.mp4
Mobile Web - Safari
mobile-safari.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mp4