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

Fix end parenthesis issue with urls in ExpensiMark #361

Merged
merged 6 commits into from
Apr 12, 2021
Merged

Fix end parenthesis issue with urls in ExpensiMark #361

merged 6 commits into from
Apr 12, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Apr 11, 2021 •
edited
Loading

Please review @Beamanator

Details of the issue can be found here Expensify/App#1656
Explanation of the solution can be found in Expensify/App#1656 (comment) and Expensify/App#1656 (comment) comments.

Fixed Issues

$ Expensify/App#1656

Tests

  1. What unit/integration tests cover your change? What auto QA tests cover your change?
    I have added more tests to support the logic here.

  2. What tests did you perform that validates your changes worked?
    I have run the unit tests over the codebase which passed.

QA

  1. What does QA need to do to validate your changes?
  • You can run the unit test and see if they passed.
  • Or, You can use the latest commit hash of merging branch and use it like
"expensify-common": "git+https://github.com/parasharrajat/expensify-common.git#dfe547cac0f1f43ba69fcbbe6c8ddb70f8c0b22d"

into Expensify.cash and then following with npm install. Now you can test various URLs in the chat as mentioned in the issue.

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

URls in chat messages.

@parasharrajat parasharrajat requested a review from a team as a code owner April 11, 2021 18:05
@MelvinBot MelvinBot requested review from joelbettner and removed request for a team April 11, 2021 18:05
@parasharrajat parasharrajat mentioned this pull request Apr 11, 2021
Closed
@Beamanator Beamanator self-requested a review April 12, 2021 10:17
Beamanator
Beamanator requested changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks fantastic, just a few picky changes :D

__tests__/ExpensiMark-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-test.js Outdated
@@ -283,3 +283,26 @@ test('Test general email link with various styles', () => {

expect(parser.replace(testString)).toBe(resultString);
});

test('Test link with brackets', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a better name could be 'Test markdown and url links with inconsistent starting and closing parens'.

Why? This subset of tests is unique because it's testing adding extra ( and ) at the beginning and end of links coming from pure urls & from markdown links.

I'm open to other names, but I think making it a bit more specific could be nice, since we already have url tests named 'Test a url ending with a closing parentheses autolinks correctly' and 'Test markdown style link with various styles'. If we make this test name super specific, this justifies why these test are in their own section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Surely, I agree.

@Beamanator
Copy link
Contributor

Beamanator commented Apr 12, 2021 •
edited
Loading

Also when you mention "You can use the latest hash of the following PR into Expensify.cash and Try different URLs as per mentioned in the issue." would you mind adding additional detail (like the hash is just the latest commit id on your branch) or showing exactly what that would end up looking like? ("expensify-common": "git+https://github.com/parasharrajat/expensify-common.git#f7483301de8d0f017e7613f203561f592ff4bf7a", then npm install)

@parasharrajat @Beamanator
Update __tests__/ExpensiMark-test.js
6a9d250 
Co-authored-by: Alex Beaman <dabeamanator@gmail.com>
@parasharrajat
Copy link
Member Author

@Beamanator Updated. Thanks.

@Beamanator Beamanator self-requested a review April 12, 2021 15:39
Beamanator
Beamanator approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks great to me, let's see what other reviewer(s) have to say :D

@joelbettner
Copy link
Contributor

Looks good to me as well.

@joelbettner joelbettner merged commit d5edd0a into Expensify:master Apr 12, 2021
@parasharrajat parasharrajat mentioned this pull request Apr 12, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator approved these changes

@joelbettner joelbettner joelbettner 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.
3 participants
@parasharrajat @Beamanator @joelbettner