Skip to content
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/14941 whitespace above block quotes is stripped away #503

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Feb 21, 2023

Fixed Issues

$ Expensify/App#14941

Tests

For all platforms

1, Compose a message like this:

normal text

> quote text

normal text

2, Post the message
3, Edit the message you posted
4, Verify that there is a blank line (and an even amount of space) above and below the block quote. The same message you sent in step 1 is shown in the editor

Additional test for IOS and Android (to ensure there is no regression bug created by the PR)

1, Compose a message like this:

normal text
> quote text
normal text

2, Post the message
3, Verify that there is no extra space displayed when used non-quoted and quoted than non-quoted text

QA

For all platforms

1, Compose a message like this:

normal text

> quote text

normal text

2, Post the message
3, Edit the message you posted
4, Verify that there is a blank line (and an even amount of space) above and below the block quote. The same message you sent in step 1 is shown in the editor

Additional test for IOS and Android (to ensure there is no regression bug created by the PR)

1, Compose a message like this:

normal text
> quote text
normal text

2, Post the message
3, Verify that there is no extra space displayed when used non-quoted and quoted than non-quoted text

Screenshots/Videos

Web
web.mp4
Mobile Web - Chrome
chrome.mp4
Mobile Web - Safari
safari.mp4
Desktop
desktop.mp4
iOS
ios.2.mp4
Android
android.1.mp4

@tienifr tienifr marked this pull request as ready for review February 23, 2023 03:10
@tienifr tienifr requested a review from a team as a code owner February 23, 2023 03:10
@melvin-bot melvin-bot bot requested review from dangrous and removed request for a team February 23, 2023 03:11
@tienifr
Copy link
Contributor Author

tienifr commented Feb 23, 2023

Hi @dangrous this PR is ready for review. Please help me check it. Thank you!

Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tests pass!

@dangrous
Copy link
Contributor

This should be good, just answering some questions before merging

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 23, 2023

good call, let's not merge yet

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 23, 2023

@tienifr could you please add screenshots for all platforms? (I'll do the same)

@tienifr
Copy link
Contributor Author

tienifr commented Feb 23, 2023

@rushatgabhane Noted. I just added screenshot for all platforms please help to check. Thanks

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

iOS

image

Android

Web

image

mWeb

image

@rushatgabhane
Copy link
Member

@dangrous we're good to merge!

the flag was already added in this PR https://github.com/Expensify/App/pull/14822/files and turned on for native code so the additional tests for iOS/Android are working

@tienifr has answered your questions Expensify/App#14941 (comment)

@dangrous dangrous merged commit fff3044 into Expensify:main Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants