Skip to content

Add method normalizeWebsiteOfUrl to format the website of url to lower case#537

Merged
MariaHCD merged 6 commits into
Expensify:mainfrom
eh2077:18415-parse-website-to-case-insensitive
May 17, 2023
Merged

Add method normalizeWebsiteOfUrl to format the website of url to lower case#537
MariaHCD merged 6 commits into
Expensify:mainfrom
eh2077:18415-parse-website-to-case-insensitive

Conversation

@eh2077

@eh2077 eh2077 commented May 15, 2023

Copy link
Copy Markdown
Contributor

This PR adds a new method normalizeWebsiteOfUrl in str.js to format the website of url to lower case. The new method is used in both link and autolink rule replacement to format the href property of an anchor tag. It can also be used in the App codebase when handling edited link.

Fixed Issues

$ Expensify/App#18514

Tests

  1. Add a comment
Google.com
  1. Click to edit the comment and verify the domain google.com of the url is in lower case in the initial draft, like
[Google.com](https://google.com)
  1. Edit the comment to
[Google.com](https://Google.com)

and save the comment
4. Verify that the link is unchanged.

Screen.Recording.2023-05-16.at.9.46.32.PM.mov

QA

N/A

@eh2077 eh2077 requested a review from a team as a code owner May 15, 2023 16:14
@melvin-bot melvin-bot Bot requested review from Gonals and removed request for a team May 15, 2023 16:14
Gonals
Gonals previously approved these changes May 16, 2023
@Gonals

Gonals commented May 16, 2023

Copy link
Copy Markdown
Contributor

Looks good, but there are some conflicts!

@MariaHCD MariaHCD self-requested a review May 16, 2023 08:29
@MariaHCD

Copy link
Copy Markdown
Contributor

@fedirjh Could you review this as well?

@MariaHCD MariaHCD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good so far. @eh2077 can you test with the App as well?

@fedirjh

fedirjh commented May 16, 2023

Copy link
Copy Markdown
Contributor

Reviewing shortly. @eh2077 could you please include App screenshots testing ?

@eh2077 eh2077 force-pushed the 18415-parse-website-to-case-insensitive branch from 8aa27e2 to fc9b7bc Compare May 16, 2023 13:47
@eh2077

eh2077 commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

@fedirjh @MariaHCD Updated App test steps and attached demo video in the PR description part, thank you.

@eh2077 eh2077 requested review from Gonals and MariaHCD May 16, 2023 15:02

@fedirjh fedirjh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good , left few comments

Comment thread lib/str.js Outdated
Comment thread lib/str.js Outdated
Comment thread __tests__/Str-test.js Outdated
Comment thread lib/ExpensiMark.js Outdated
eh2077 and others added 4 commits May 16, 2023 23:56
Co-authored-by: Fedi Rajhi <fedirjh@gmail.com>
Co-authored-by: Fedi Rajhi <fedirjh@gmail.com>
Co-authored-by: Fedi Rajhi <fedirjh@gmail.com>
@eh2077 eh2077 requested a review from fedirjh May 16, 2023 16:03
fedirjh
fedirjh previously approved these changes May 16, 2023

@fedirjh fedirjh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

cc @MariaHCD

Gonals
Gonals previously approved these changes May 17, 2023
@eh2077

eh2077 commented May 17, 2023

Copy link
Copy Markdown
Contributor Author

@MariaHCD Friendly bump! Would you like to have a look at this PR? Thank you!

@MariaHCD MariaHCD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! One minor comment and I'm just going to quickly test this on OldDot to ensure nothing breaks there.

Comment thread lib/str.js Outdated

@MariaHCD MariaHCD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did some testing on OldDot, looks good!

Screenshot 2023-05-17 at 4 17 11 PM

Co-authored-by: Maria D'Costa <mariahcdcosta@gmail.com>
@eh2077 eh2077 dismissed stale reviews from Gonals and fedirjh via 58224a6 May 17, 2023 12:29
@eh2077 eh2077 requested review from MariaHCD and fedirjh May 17, 2023 12:31
@MariaHCD MariaHCD merged commit c20d572 into Expensify:main May 17, 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.

4 participants