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

Use linkifyjs for better url extraction lgic #569

Merged
merged 2 commits into from
Apr 14, 2024

Conversation

Tucchhaa
Copy link
Contributor

@Tucchhaa
Copy link
Contributor Author

Tucchhaa commented Apr 14, 2024

I also could replace all calls of linkify() and nl2br() to by element , but decided to do as less changes as possible. If it's needed, please ask me and I'll do that

@coveralls
Copy link

Coverage Status

coverage: 71.231% (-0.02%) from 71.246%
when pulling 9f2dd8c on Tucchhaa:add_linkifyjs
into 098ec82 on cofacts:master.

@coveralls
Copy link

Coverage Status

coverage: 71.231% (-0.02%) from 71.246%
when pulling 9f2dd8c on Tucchhaa:add_linkifyjs
into 098ec82 on cofacts:master.

@MrOrz MrOrz self-requested a review April 14, 2024 09:32
@MrOrz MrOrz linked an issue Apr 14, 2024 that may be closed by this pull request
@MrOrz
Copy link
Member

MrOrz commented Apr 14, 2024

Thank you for your PR @Tucchhaa ! I have deployed your change to staging sites ( https://dev.cofacts.tw/ ) and it will be tested during the dev meeting tonight.

Regarding "replace all calls of linkify() and nl2br() to by element" would you explain more on what you want to achieve?

@Tucchhaa
Copy link
Contributor Author

Thank you for your PR @Tucchhaa ! I have deployed your change to staging sites ( https://dev.cofacts.tw/ ) and it will be tested during the dev meeting tonight.

Regarding "replace all calls of linkify() and nl2br() to by element" would you explain more on what you want to achieve?

It wouldn't be any functional change, just a small refactoring. I've notived that nl2br() and linkify() are called together almost always, and we could replace them just by tag.

@MrOrz
Copy link
Member

MrOrz commented Apr 14, 2024

Thanks for analyzing the code and make the proposal. Personally I think nl2br and linkify are two separate functions, while always used together, it can be more flexible to keep them separated.

Let me merge this PR and ship to production 🚀

@MrOrz MrOrz merged commit b8d64a7 into cofacts:master Apr 14, 2024
9 of 10 checks passed
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.

Better URL extraction logic
3 participants