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

Fix autolink isn't recognized when put before img markdown #782

Conversation

bernhardoj
Copy link
Contributor

@bernhardoj bernhardoj commented Aug 15, 2024

Fixed Issues

$ Expensify/App#46491

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    A unit test is updated

  2. What tests did you perform that validates your changed worked?

  • Send a text + attachment message
  • Edit the message
  • Add a link (e.g., google.com) above the image markdown
  • Verify the link text is rendered as a link
  • Save the edit
  • Verify the link message is saved as a link

To be able to test the live markdown, update the regex in node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js

Android: Native
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4

QA

  1. What does QA need to do to validate your changes?
    Same as Tests
  2. What areas to they need to test for regressions?
    Same as Tests

@bernhardoj bernhardoj requested a review from a team as a code owner August 15, 2024 11:31
@melvin-bot melvin-bot bot requested review from danieldoglas and removed request for a team August 15, 2024 11:31
@bernhardoj
Copy link
Contributor Author

I'm not sure how can we test this visually, but if we convert

![# fake-heading *bold* _italic_ ~strike~ `code`](https://example.com/image.png)

using the ExpensiMark, the result is

<img src="https://example.com/image.png" alt="# fake-heading *bold* _italic_ ~strike~ &#x60;code&#x60;" />

@bernhardoj
Copy link
Contributor Author

Looks like changing node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js for native doesn't work anymore. We can test the live markdown later in the live markdown repo..

@danieldoglas danieldoglas requested review from dangrous and removed request for danieldoglas August 15, 2024 15:15
@danieldoglas
Copy link
Contributor

adding @dangrous since he's the CME for this issue. also cc @getusha

@dangrous
Copy link
Contributor

thanks @danieldoglas! @getusha do you want to go through the testing on this one? I'm still seeing the bug but I might have incorrectly linked the module, so I'll try again in a bit

@getusha
Copy link
Contributor

getusha commented Aug 16, 2024

It's working fine for me

Screen.Recording.2024-08-16.at.1.14.11.in.the.afternoon.mov

I'll test it on native as well

Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

okay finally got this to work. Looks good to me! Can we also please double check that we're not reintroducing #661 (comment) ? Thanks!

@bernhardoj
Copy link
Contributor Author

@dangrous I commented about that here

@dangrous
Copy link
Contributor

oh i missed that. Okay great! (side note: do we not show alt text of images? It's not showing up for me.)

@dangrous dangrous merged commit e89eff9 into Expensify:main Aug 19, 2024
6 checks passed
Copy link

🚀Published to npm in v2.0.76

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