Skip to content

feat: manage link in package description#147

Merged
danielroe merged 7 commits intonpmx-dev:mainfrom
jycouet:feat/pkg-description-link
Jan 26, 2026
Merged

feat: manage link in package description#147
danielroe merged 7 commits intonpmx-dev:mainfrom
jycouet:feat/pkg-description-link

Conversation

@jycouet
Copy link
Copy Markdown
Contributor

@jycouet jycouet commented Jan 26, 2026

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 26, 2026

@jycouet is attempting to deploy a commit to the danielroe Team on Vercel.

A member of the Team first needs to authorize it.

Comment thread app/components/MarkdownText.vue Outdated
// Links: [text](url) - only allow http, https, mailto
html = html.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_match, text, url) => {
const decodedUrl = url.replace(/&/g, '&')
if (/^(https?:|mailto:)/i.test(decodedUrl)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is probably not sufficient from a security point of view

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did an update with

  • URL class
  • manage only https & mailto

Let me know what you have in mind, I can do more researches 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good to me!

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
npmx.dev Ready Ready Preview, Comment Jan 26, 2026 11:30pm

Request Review

Comment thread app/components/MarkdownText.vue Outdated
@danielroe
Copy link
Copy Markdown
Member

oh - one more issue - we have to avoid rendering a link when it's in a <PackageCard> e.g. in a search result.

@jycouet
Copy link
Copy Markdown
Contributor Author

jycouet commented Jan 26, 2026

oh - one more issue - we have to avoid rendering a link when it's in a <PackageCard> e.g. in a search result.

You mean here
image

Even if it's not clickable, it's nicer no?

@jycouet
Copy link
Copy Markdown
Contributor Author

jycouet commented Jan 26, 2026

Or having the text underlined, but not a link in a "semi-plain" mode? :D

@danielroe
Copy link
Copy Markdown
Member

I'd be up for it being underlined - but ... that does make the user think they can click it, right? 🤔

@jycouet
Copy link
Copy Markdown
Contributor Author

jycouet commented Jan 26, 2026

Or bold? Or different underline?
Just to not show [ ] ( )

@danielroe
Copy link
Copy Markdown
Member

this is what it currently displays (no special chars):

SCR-20260126-uigf

@jycouet
Copy link
Copy Markdown
Contributor Author

jycouet commented Jan 26, 2026

What about ?
A

if (props.plain) {
  return `<span class="color-fg">${text}</span>`;
}
image

B

<strong>${text}</span>`;
image

I think I like A, but I have to say that I'm a very be designer :D

@danielroe
Copy link
Copy Markdown
Member

the problem is - this is only meant to be a description, not to have its own CTA. we shouldn't highlight that CTA for that reason. it should be as muted as the rest of the description. that's a bit strange, but that's because it is being misused a bit by that package, imo...

Comment thread app/components/MarkdownText.vue Outdated
@jycouet
Copy link
Copy Markdown
Contributor Author

jycouet commented Jan 26, 2026

the problem is - this is only meant to be a description, not to have its own CTA. we shouldn't highlight that CTA for that reason. it should be as muted as the rest of the description. that's a bit strange, but that's because it is being misused a bit by that package, imo...

I get it, let's keep it like this 👍

@danielroe danielroe merged commit c79a304 into npmx-dev:main Jan 26, 2026
1 check failed
vinnymac pushed a commit to vinnymac/npmx.dev that referenced this pull request Jan 28, 2026
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.

3 participants