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

Update regex of italic #526

Merged
merged 5 commits into from
May 5, 2023
Merged

Update regex of italic #526

merged 5 commits into from
May 5, 2023

Conversation

dukenv0307
Copy link
Contributor

@dukenv0307 dukenv0307 commented May 3, 2023

Details

When entering ___ (3 underscores), the italic characters (underscores) got parsed and only leave one underscore after parsed.
It has 3 changes compared to the current italic regex we're using:

  • Replace (?!\s) by (?![\s_]), so that it doesn't match _ at the beginning of the content

  • Replace \S by [^\s_], so that it doesn't match _ at the end of the content

  • Replace the word boundary \b by a regex at the beginning and end ([^\W_]?) and (?![^\W_]) so that the word boundary will also exclude the _ as word, so that cases like content and content will work correctly.

Also we need to update this replacement so that it will not replace if the match starts with a word character (this logic works with ([^\W_]?))

Fixed Issues

$ Expensify/App#17665
PROPOSAL: Expensify/App#17665 (comment)

Tests

To test, we need to update the hash commit of expensify-common package in App repo into 6a781aa8f027d87f78e5dd808759795631fc30e5 like this:
"expensify-common": "git+ssh://git@github.com/Expensify/expensifycommon.git#6a781aa8f027d87f78e5dd808759795631fc30e5"

Open any chat and send these messages and verify that

  1. ___ --> Verify that this message isn't parsed
  2. _italic__ --> Verify that this message is parsed
  3. __italic_ --> Verify that this message is parsed
  4. _italic_another_italic_ --> Verify that this message is parsed
  5. _italic_ --> Verify that this message is parsed
  6. s_italic --> Verify that this message isn't parsed
  7. italic_s --> Verify that this message isn't parsed
_italic

italic
italic_

--> Verify that this message is parsed
9.

_
_
_

--> Verify that this message isn't parsed
10.

_
_test
_

--> Verify that this message isn't parsed

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Open any chat
  2. Send these messages and verify that
    ___ --> Verify that this message isn't parsed
    _italic__ --> Verify that this message is parsed
    __italic_ --> Verify that this message is parsed
    _italic_another_italic_ --> Verify that this message is parsed
    _italic_ --> Verify that this message is parsed
    s_italic --> Verify that this message isn't parsed
    italic_s --> Verify that this message isn't parsed
  • Verify that no errors appear in the JS console

Screenshots/Videos

Web Screenshot 2023-05-03 at 16 32 56
Mobile Web - Chrome Screenshot 2023-05-03 at 16 38 15
Mobile Web - Safari Screenshot 2023-05-03 at 16 38 36
Desktop Screenshot 2023-05-03 at 16 42 28
iOS Screenshot 2023-05-03 at 16 59 57
Android Screenshot 2023-05-03 at 17 11 05

@dukenv0307 dukenv0307 requested a review from a team as a code owner May 3, 2023 09:23
@github-actions
Copy link

github-actions bot commented May 3, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from alex-mechler and removed request for a team May 3, 2023 09:23
@dukenv0307
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@dukenv0307
Copy link
Contributor Author

recheck

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

@dukenv0307

  • Please add more tests for multiline syntax.
  • Add unit tests for the new changes to the regex.

Do you think we should also update the bold regex?

@dukenv0307
Copy link
Contributor Author

@parasharrajat Thanks so much for your reminder. I just updated, please help to check again

@parasharrajat
Copy link
Member

@alex-mechler can you please trigger checks?

@parasharrajat
Copy link
Member

parasharrajat commented May 4, 2023

@dukenv0307 I will suggest that you create the PR on the Expensify/APP repo and use the latest commit hash from this PR. We can test it meanwhile this is being reviewed and merged. Once this is merged, we can update the hash there and close that as well. This will speed up the process.

@dukenv0307
Copy link
Contributor Author

@parasharrajat The PR in App repo is ready

parasharrajat
parasharrajat previously approved these changes May 4, 2023
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -139,8 +139,8 @@ export default class ExpensiMark {
* Use [\s\S]* instead of .* to match newline
*/
name: 'italic',
regex: /(?!_blank")\b_((?!\s)[\s\S]*?\S)_\b(?![^<]*(<\/pre>|<\/code>|<\/a>|_blank))/g,
replacement: (match, g1) => (g1.includes('<pre>') ? match : `<em>${g1}</em>`),
regex: /(?!_blank")([^\W_]?)_((?![\s_])[\s\S]*?[^\s_])_(?![^\W_])(?![^<]*(<\/pre>|<\/code>|<\/a>|_blank))/g,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this capture group here. We aren't doing anything with the captured characters, and it would let us avoid changing the replacement. The test suite passes locally, and it captures the same test strings mentioned in the issue without this group

Suggested change
regex: /(?!_blank")([^\W_]?)_((?![\s_])[\s\S]*?[^\s_])_(?![^\W_])(?![^<]*(<\/pre>|<\/code>|<\/a>|_blank))/g,
regex: /(?!_blank")[^\W_]?_((?![\s_])[\s\S]*?[^\s_])_(?![^\W_])(?![^<]*(<\/pre>|<\/code>|<\/a>|_blank))/g,

@dukenv0307
Copy link
Contributor Author

dukenv0307 commented May 5, 2023

@alex-mechler I updated and tested again, it works well. Also updated new hash in App repo. Please help to check again.
cc @parasharrajat

Copy link
Contributor

@alex-mechler alex-mechler left a comment

Choose a reason for hiding this comment

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

LGTM and tested well, thanks for the quick updates!

@alex-mechler
Copy link
Contributor

Going to merge this since @parasharrajat had already approved

@alex-mechler alex-mechler merged commit cb58369 into Expensify:main May 5, 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