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

Allow indented text in quotes #673

Merged
merged 5 commits into from
Apr 2, 2024
Merged

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Mar 28, 2024

This PR enables indented text to show as is (do not trim the spaces) in quotes.

Fixed Issues

$ Expensify/App#38156

Tests

1. What unit/integration tests cover your change? What autoQA tests cover your change?

The ExpensiMark-HTML-test.js is updated to include the new test cases and modify existing tests to match with new expected behaviors.

2. What tests did you perform that validates your changed worked?

  • Run the unit test.
  • Test in Expensify App follow these steps:
  1. Send a quote message with indentation:
> No indent
>     Indented 4
  1. Verify the Indented 4 line shows with 4-space indentation in quote

QA

1. What does QA need to do to validate your changes?

Test in Expensify App follow these steps:

  1. Send a quote message with indentation:
> No indent
>     Indented 4
  1. Verify the Indented 4 line shows with 4-space indentation in quote

2. What areas to they need to test for regressions?

Screenshots

Screen.Recording.2024-03-29.at.02.13.47-compressed.mov

@s77rt
Copy link
Contributor

s77rt commented Mar 28, 2024

Inconsistency, send the following message:

> k
>>       hi         
>       hi there        
> k

Notice that there are spaces after both hi and hi there. But when the message is sent we are trimming the spaces only on the nested quote (the hi message)

Screenshot 2024-03-28 at 11 11 57 PM .

@tienifr
Copy link
Contributor Author

tienifr commented Mar 29, 2024

@s77rt I want to confirm the expected behavior:

  1. Allow leading spaces; trim the trailing and the ones between >
  2. Allow spaces in both ends; trim the spaces between >

1️⃣ or 2️⃣?

@s77rt
Copy link
Contributor

s77rt commented Mar 29, 2024

@tienifr I don't have a strong preference, as long as we have consistency it should be good enough. Perhaps 2️⃣ is a better fit here

@s77rt
Copy link
Contributor

s77rt commented Mar 30, 2024

@tienifr Any progress on this?

@tienifr
Copy link
Contributor Author

tienifr commented Apr 1, 2024

@s77rt I forgot about the case where there're trailing spaces at the end of the whole quote. If we follow 2️⃣ above, we can opt in to keep (1) or trim (2) these spaces. For example:

>⎵⎵⎵quote⎵
>⎵⎵trailing⎵⎵

Output can either be

(1) <blockquote>⎵⎵quote⎵<br />⎵trailing⎵⎵</blockquote>

or

(2) <blockquote>⎵⎵quote⎵<br />⎵trailing</blockquote>

I have solutions for both but (2) will add one more conditional statements.

@tienifr
Copy link
Contributor Author

tienifr commented Apr 1, 2024

  1. Allow leading spaces; trim the trailing and the ones between &gt;

After all, I think this is the best option 😄 We can avoid increased code complexity, maintain consistency but still achieve the expected behavior. Current implementation also trim trailing spaces.

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
tienifr and others added 2 commits April 2, 2024 11:51
Co-authored-by: Abdelhafidh Belalia <[email protected]>
Co-authored-by: Abdelhafidh Belalia <[email protected]>
@tienifr
Copy link
Contributor Author

tienifr commented Apr 2, 2024

So we still keep both trailing and leading spaces, right?

@Julesssss
Copy link
Contributor

So we still keep both trailing and leading spaces, right?

@s77rt from your conversation above this sounds the best option. Can you please confirm so we can finish up this PR. Thanks

@s77rt
Copy link
Contributor

s77rt commented Apr 2, 2024

Yes, we won't alter user input on quotes

@s77rt
Copy link
Contributor

s77rt commented Apr 2, 2024

cc @Julesssss

@Julesssss
Copy link
Contributor

Testing to occur when the common library is bumped in App

@Julesssss Julesssss merged commit 8dd2cb4 into Expensify:main Apr 2, 2024
5 checks passed
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