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

Issue 26490 remove replacebr #606

Merged
merged 7 commits into from
Nov 21, 2023

Conversation

nikos-amofa
Copy link
Contributor

@nikos-amofa nikos-amofa commented Nov 17, 2023

This PR:

  • removes the replacebr rule option inside ExpensiMark.rules

Fixed Issues

$ Expensify/App#26490
$ Expensify/App#16982

Tests

Unit testing

  1. Updated existing tests which are affected by this change to pass
  2. Added new unit testing

Testing in the expensify-app

  1. On your local dev environment of expensify-app, open node_modules/expensify-common/lib/ExpensiMark.js file
  2. Remove replacebr rule
  3. Run the app and test with the following recordings

Test Strings

```
code1
```
text
```
code2
```
|
```
code
```
```
code

```
message1```code```message2
message1 ``` code ``` message2
message1
```code```
message2
message1
``` code ```
message2
message1
```

code
```
message2
message1
```

code

```
message2
message1
```


code

```
message2
message1
```

code


```
message2
Screen.Recording.2023-10-14.at.11.24.25.AM.mov
Screen.Recording.2023-10-14.at.11.25.23.AM.mov
Screen.Recording.2023-11-15.at.9.59.11.AM.mov
Screen.Recording.2023-11-15.at.10.00.14.AM.mov
Screen.Recording.2023-11-15.at.10.00.58.AM.mov

QA

Same as test

@nikos-amofa nikos-amofa requested a review from a team as a code owner November 17, 2023 13:38
@melvin-bot melvin-bot bot requested review from danieldoglas and removed request for a team November 17, 2023 13:38
@0xmiros
Copy link
Contributor

0xmiros commented Nov 17, 2023

Please add test strings in Tests step for easier test. Example: #573.

@nikos-amofa
Copy link
Contributor Author

Please add test strings in Tests step for easier test. Example: #573.

Thanks, addressed by 7992a00

@0xmiros
Copy link
Contributor

0xmiros commented Nov 17, 2023

Like this:

Screenshot 2023-11-17 at 5 17 03 pm

@0xmiros
Copy link
Contributor

0xmiros commented Nov 18, 2023

Add some more test cases with no line break, multiple line breaks, with/without space
i.e.

message1```code```message2

message1 ``` code ``` message2

message1
```code```
message2

message1
``` code ```
message2


message1
```

code
```
message2


message1
```

code

```
message2


message1
```


code

```
message2


message1
```

code


```
message2

@nikos-amofa
Copy link
Contributor Author

@0xmiroslav Do I need to add those cases to the unit testing or to the test strings in the PR description?

@0xmiros
Copy link
Contributor

0xmiros commented Nov 19, 2023

@0xmiroslav Do I need to add those cases to the unit testing or to the test strings in the PR description?

Both

@danieldoglas
Copy link
Contributor

@0xmiroslav can you please do the review checklist on this one?

Thanks!

@nikos-amofa
Copy link
Contributor Author

@0xmiroslav After this PR is merged, do I need to create another PR in the App to update the expensify-common package version? What will be measured as the days for the PR merged - this PR or another PR for App?
Upwork contract says that if I can get merged within 3 days, I can get 50% bonus, but if merged after 9 days, I will get penalty

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2023

Just to confirm that this is not bug.

message1```code```message2

In edit mode:

message1```
code
```
message2

@nikos-amofa
Copy link
Contributor Author

@0xmiroslav Yes, we can see the same result in the current staging environment.
I assume if we want to fix it, we will need to create another issue and address it, but as we don't really want the edit mode to be exactly as the original text, it can be decided by the product team.

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2023

Screenshots/Videos

Android: Native
android.mov
Android: mWeb Chrome
mchrome.mov
iOS: Native
ios.mov
iOS: mWeb Safari
msafari.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov

Copy link
Contributor

@0xmiros 0xmiros left a comment

Choose a reason for hiding this comment

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

LGTM!

@srikarparsi all yours

@nikos-amofa
Copy link
Contributor Author

@danieldoglas Is there anything I need to do to merge this PR?

@danieldoglas danieldoglas merged commit ee14b32 into Expensify:main Nov 21, 2023
5 checks passed
@nikos-amofa
Copy link
Contributor Author

@danieldoglas Can you please update the version of expensify-common?

@danieldoglas
Copy link
Contributor

@nikosamofa You just need to follow this: https://github.com/Expensify/expensify-common?tab=readme-ov-file#deploying-a-change-expensify-only on the app repo!

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