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

URL does not get highlighted properly in Expensify.cash #1234

Closed
roryabraham opened this issue Jan 13, 2021 · 11 comments · Fixed by Expensify/expensify-common#327
Closed

URL does not get highlighted properly in Expensify.cash #1234

roryabraham opened this issue Jan 13, 2021 · 11 comments · Fixed by Expensify/expensify-common#327
Assignees

Comments

@roryabraham
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding!


Platform

All platforms:

  1. Web
  2. macOS Desktop
  3. Android
  4. iOS
  5. Mobile web

Version

1.0.1-341

Action Performed (reproducible steps):

  1. Paste the following into an Expensify.cash chat:
Check out [this link](https://www.expensify.com/_devportal/tools/logSearch/#query=request_id:(%22Ufjjim%22)+AND+timestamp:[2021-01-08T03:48:10.389Z+TO+2021-01-08T05:48:10.389Z]&index=logs_expensify-008878)!
  1. Send the message.

Expected Result

The link is created properly and looks like this:

image

Actual Result

The URL is not properly highlighted and the link is not created properly.

image

Solution notes

The code that processes markdown is actually not in Expensify/Expensify.cash, but Expensify/expensify-common.

Add a unit test for this URL in ExpensiMark-test.js and then fix ExpensiMark to handle the URL properly.

A solution will not be considered complete unless it:

  1. Creates an automated unit test to cover this URL and potentially others like it,
  2. Provides videos of the working URL on all 5 platforms of Expensify.cash (Web, macOS Desktop, iOS, Android, mobile web). These must be video screen recordings and not static screenshots, so that we know that the correct URL was used to create the link, and that the link was created properly.
@roryabraham
Copy link
Contributor Author

Posted to Upwork: https://www.upwork.com/jobs/~01c9f994a1995338e5

@roryabraham
Copy link
Contributor Author

Additionally, would like to see that the solution also works for this URL: http://necolas.github.io/react-native-web/docs/?path=/docs/components-pressable--disabled

@parasharrajat
Copy link
Member

is there any restriction of using regexp only?

@roryabraham
Copy link
Contributor Author

What else would you use? These are the regexes we have currently

@Dal-Papa
Copy link

I was about to create a similar issue for the :~:text= syntax from Google Chrome (like here)
Screen Shot 2021-01-12 at 10 15 35

@parasharrajat
Copy link
Member

@roryabraham Sorry for the late reply. I checked it. After making some adjustments to the URL Regex we can cover your case and other cases and also @Dal-Papa case.
But there is one regression.

https://github.com/Expensify/Expensify.cash/issues/1234#:~:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash

The above URL is being converted to link successfully but Strike conversion is swapping ~ with <del>.

Can we adjust the strike regex to discard match when we found :~:text?

@Dal-Papa
Copy link

@parasharrajat : Should we rather fix it so that if the <del> doesn't have an ending ~, then it doesn't strike ?

@parasharrajat
Copy link
Member

parasharrajat commented Jan 19, 2021

@Dal-Papa so

https://github.com/Expensify/Expensify.cash/issues/1234#:~:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash 

is converted to

<a href="https://github.com/Expensify/Expensify.cash/issues/1234#:~:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash" target="_blank">https://github.com/Expensify/Expensify.cash/issues/1234#:~:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash</a>

which then will be replaced with <del> in place of ~

<a href=\"https://github.com/Expensify/Expensify.cash/issues/1234#:<del>:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash\" target=\"_blank\">https://github.com/Expensify/Expensify.cash/issues/1234#:</del>:text=The%20code%20that%20processes%20markdown%20is%20actually%20not%20in%20Expensify/Expensify.cash</a>

I see a possible solution that we can adopt is to check if ~ is between <a and > thus do not parse it as strike token.

@lucas-neuhaus-dev
Copy link
Contributor

@roryabraham
Is this issue still open for grabs?

I did some tests and I believe I can create a PR with everything that is required on the deliverable (including the additions on the comments section).

@kadiealexander
Copy link
Contributor

Hey @lucas-neuhaus-dev! Please review the rules here for submitting a Pull Request. Looking forward to hearing your proposal!

@lucas-neuhaus-dev
Copy link
Contributor

lucas-neuhaus-dev commented Jan 28, 2021

Hi @kadiealexander, sorry I forgot to comment here, I created the PR this afternoon.

Here it is: Expensify/expensify-common#327.
I will mention you in the description there as well.

Edit: Now I realized (even having done it 4 times before) that I jumped the proposal step and went direct for the PR.
My bad. But I think it is better to keep the discussion on the PR page now, and I can fix any possible review.

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 a pull request may close this issue.

5 participants