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.

Sign up for GitHub

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

Jump to bottom

Update strikethrough replace rule to fix multiple tilde error #569

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Update strikethrough replace rule to fix multiple tilde error #569

merged 4 commits into from
Sep 11, 2023

Conversation

akamefi202
Copy link
Contributor

@akamefi202 akamefi202 commented Aug 31, 2023 •
edited
Loading

Update strikethrough replace rule to fix multiple tilde error when transition from text to html.
To make sure that <del> tag contains content text, if it's wrapped by multiple tilde characters.
Currently, <del> tag contains 2 tilde characters too after transition.

Fixed Issues

Expensify/App#25693

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    In ExpensiMark-HTML-test.js file, test named "Test strikethrough with multiple tilde characters" works correctly.
  2. What tests did you perform that validates your changed worked?
    Checked if the updated strikethrough rule only wraps content text inside multiple tilde characters.

QA

  1. What does QA need to do to validate your changes?
    Run the new test cases.
  2. What areas to they need to test for regressions?
    N/A

@akamefi202 akamefi202 requested a review from a team as a code owner August 31, 2023 06:23
@melvin-bot melvin-bot bot requested review from neil-marcellini and removed request for a team August 31, 2023 06:24
@akamefi202 akamefi202 mentioned this pull request Aug 31, 2023
Closed
6 tasks
parasharrajat
parasharrajat reviewed Aug 31, 2023
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member

parasharrajat commented Aug 31, 2023 •
edited
Loading

Fix the PR title. It should explain your changes. Please use better commit messages which explain the code.

@akamefi202 akamefi202 changed the title first commit Update strikethrough replace rule to fix multiple tilde error Aug 31, 2023
@akamefi202
Copy link
Contributor Author

akamefi202 commented Aug 31, 2023 •
edited
Loading

@parasharrajat I updated the PR title, commit message and description.

@akamefi202 akamefi202 force-pushed the strikethrough-markdown branch from a164b47 to 7227665 Compare August 31, 2023 09:40
parasharrajat
parasharrajat approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

cc: @neil-marcellini

neil-marcellini
neil-marcellini requested changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

I think we should also test html to text and html to markdown to make sure that all works too.

I don't really understand the full regex, so let's be sure to also test the steps from the PR where this was last changed.

__tests__/ExpensiMark-HTML-test.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member

Yes for #461, we added a unit test.

@akamefi202
Copy link
Contributor Author

akamefi202 commented Aug 31, 2023 •
edited
Loading

I think we should also test html to text and html to markdown to make sure that all works too.

I think previous test cases include test for this already.

@akamefi202
Copy link
Contributor Author

@parasharrajat @neil-marcellini I pushed new commit for required changes above.

@neil-marcellini neil-marcellini changed the title Update strikethrough replace rule to fix multiple tilde error [HOLD merge freeze] Update strikethrough replace rule to fix multiple tilde error Sep 1, 2023
@akamefi202
Copy link
Contributor Author

@tylerkaraszewski @parasharrajat @neil-marcellini Can we merge this PR if it is approved?

@tylerkaraszewski
Copy link
Contributor

Not while it says "hold merge freeze" in the title. We are still waiting on the end of that freeze.

@akamefi202
Copy link
Contributor Author

@tylerkaraszewski @parasharrajat @neil-marcellini Merge freeze is over now. Could you please merge this PR soon?

@tylerkaraszewski tylerkaraszewski changed the title [HOLD merge freeze] Update strikethrough replace rule to fix multiple tilde error Update strikethrough replace rule to fix multiple tilde error Sep 11, 2023
@tylerkaraszewski tylerkaraszewski merged commit 35bff86 into Expensify:main Sep 11, 2023
3 checks passed
@akamefi202 akamefi202 deleted the strikethrough-markdown branch September 11, 2023 15:28
@akamefi202 akamefi202 mentioned this pull request Sep 11, 2023
Merged
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neil-marcellini neil-marcellini neil-marcellini approved these changes

@parasharrajat parasharrajat parasharrajat approved these changes

@tylerkaraszewski tylerkaraszewski tylerkaraszewski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akamefi202 @parasharrajat @tylerkaraszewski @neil-marcellini