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

Linkify comments #504

Merged
merged 12 commits into from
Dec 21, 2023
Merged

Linkify comments #504

merged 12 commits into from
Dec 21, 2023

Conversation

aakash2330
Copy link
Contributor

@aakash2330 aakash2330 commented Dec 17, 2023

Hyperlink Links in Comments #431

Turn Links within comments clickable .
(Please ignore the first commit )

Copy link

vercel bot commented Dec 17, 2023

@aakash2330 is attempting to deploy a commit to the Elegance Team on Vercel.

A member of the Team first needs to authorize it.

const parts = text.split(urlPattern);
return parts.map((part, index) => {
if (part.match(urlPattern)) {
return <a href={part} key={index} target="_blank" rel="noopener noreferrer" className="hover:underline">{part}</a>;
Copy link

@tuffz tuffz Dec 17, 2023

Choose a reason for hiding this comment

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

I would also recommend adding a "nofollow" to the rel attribute to avoid link spammers.

Copy link
Contributor

@unrenamed unrenamed left a comment

Choose a reason for hiding this comment

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

Hey @aakash2330, excited to help with your PR code review!

  1. Thinking about making the <LinkifyText /> component even more modular by moving it to the packages/ui module. What do you think? Also, we can tweak the filename to match the component name, maybe something like linkify-text.

  2. Noticed the Hyperlink Links in Comments #431 issue author mentioned an ideal scenario: allowing URLs without protocols to transform into hyperlinks.

    ideally not even requiring entering https:// but open to having that if it makes this easier

    Currently, it seems to support only http(s) protocols. Any challenges with this? Maybe check out existing solutions like linkify.js.org? Let me know your thoughts! 😊

@aakash2330
Copy link
Contributor Author

Hey @tuffz @unrenamed thanks for reviewing my pr .

1 . Added the "nofollow" to the rel attribute as suggested by @tuffz .
2 . Tweaked the filename to match the component name (linkify-comments) and moved it to packages/ui module .
3. Want to get up a heads up from the maintainers before including a third party library that's why I didn't include it in the first place .

Copy link

vercel bot commented Dec 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 3:56am

@steven-tey
Copy link
Collaborator

Thanks a lot for the PR @aakash2330 + feedback from @tuffz & @unrenamed 🙏

re: linkifyjs – it looks lightweight enough, let's use it 😄 (and rely on @SocketDev for 3rd party deps security 🔒)

Copy link

socket-security bot commented Dec 19, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@aakash2330
Copy link
Contributor Author

@steven-tey added linkifyjs to support more URL protocols as well as allowing URLs without protocols to transform into hyperlinks.

@steven-tey
Copy link
Collaborator

Great work on this PR @aakash2330 – also shoutout to @unrenamed for the linkify tip as well!

Will be merging this now – also updated our changelog & help docs to include this too :)

@steven-tey steven-tey merged commit f2662d7 into dubinc:main Dec 21, 2023
3 of 4 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.

None yet

4 participants