- Notifications
You must be signed in to change notification settings - Fork926
chore: add link component#16156
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
site/src/components/Link/Link.tsx Outdated
| ({ className, text, size, ...props }, ref) => { | ||
| return ( | ||
| <a className={cn(linkVariants({ size }), className)} ref={ref} {...props}> | ||
| {text} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need a non-breaking space here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cc.:@jaaydenh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm guessing that we're adding spacing between the text and icon, but I feel like flexbox would give better control over that and have fewer edge cases around work wrapping at smaller screen sizes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There needs to be some way to put a space between the text and the icon. It there a better alternative to add a space?
site/src/components/Link/Link.tsx Outdated
| { | ||
| variants: { | ||
| size: { | ||
| lg: "text-sm mb-px", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want a link to exert margins on nearby elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cc.:@jaaydenh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the margin bottom was leftover from some experimentation and cane be removed
@Parkreiner@jaaydenh I made some minor refactors and style tweaks. Additionally, I added a story to test the link within inline text. |
Hey folks, since the tests and stories are all good, I'll go ahead and merge this to unblock my work on another feature. If there's any improvement we absolutely need, please let me know, and I'll address it in my next PR. |
2052b55 intomainUh oh!
There was an error while loading.Please reload this page.
| variants: { | ||
| size: { | ||
| lg: "text-sm gap-[2px] [&_svg]:size-icon-sm [&_svg]:p-[2px] leading-6", | ||
| sm: "text-xs gap-1 [&_svg]:size-icon-xs [&_svg]:p-[1px] leading-[18px]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why usep-[1px] instead ofp-px?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't knowp-px was a thing.
Component based onLink design