feat: add Link component to next - #4601
Conversation
|
@claude review |
|
Claude finished @pomfrida's task in 2m 1s —— View job Code Review: Link Component
Overall a clean, well-structured component that follows EDS 2.0 conventions. A couple of things worth addressing before merging: Bug: Icon color mismatch in standalone default state
.eds-link[data-variant='standalone'] > svg {
color: var(--eds-color-text-strong);
}In the default state, the link text and If the design intent is for the icon and text to always be the same colour, removing the explicit icon rule lets the icon inherit
Minor: Missing CSS import in
|
ddc3b54 to
99756dd
Compare
|
@claude review |
|
Claude finished @pomfrida's task in 1m 57s —— View job Code Review: Link Component
All items raised in the previous two reviews have been addressed. The component is in good shape — one open question on icon colour intent, and a minor consistency note. Open question: Icon colour in default state
.eds-link[data-variant='standalone'] .eds-icon {
color: var(--eds-color-text-strong);
}In the default (non-hover) state, the link text and This could be intentional (icon acting as a neutral anchor while the text signals the link colour), but it's worth confirming it matches the Figma design intent. If it should match, removing the rule lets the icon inherit Minor: CSS import pattern is inconsistent (non-blocking)
What's been addressed since the first review
What looks good
· Branch |
| /* Storybook color scheme wrapper */ | ||
| .eds-storybook-wrapper { | ||
| background-color: var(--eds-color-bg-neutral-surface); | ||
| color: var(--eds-color-text-strong); |
There was a problem hiding this comment.
Added this to support dark mode in stories (for text)
millus
left a comment
There was a problem hiding this comment.
Looks really good! So clean and easy to read 🤩 Just some minor things here and there that me (and claude) have noted down 🤖
| .eds-link { | ||
| display: inline; | ||
| } |
There was a problem hiding this comment.
Can remove this one part if we make data-font-family conditional, so it will not be effective for inline. If not, just ignore this comment and keep as is.
| color: var(--eds-color-text-link); | ||
| text-decoration: underline; | ||
| text-decoration-thickness: var(--eds-sizing-stroke-thin); | ||
| text-underline-offset: var(--eds-sizing-stroke-thick); |
There was a problem hiding this comment.
Why is --eds-sizing-stroke-thick used for spacing? Shouldn’t this be --eds-spacing-vertical-4xs?
| border-radius: var(--eds-spacing-border-radius-rounded); | ||
| color: var(--eds-color-text-strong); | ||
| outline: var(--eds-sizing-stroke-thin) solid var(--eds-color-border-focus); | ||
| outline-offset: calc(var(--eds-sizing-stroke-thick) * 2); |
There was a problem hiding this comment.
Shouldn’t this be --eds-spacing-vertical-3xs?
| content: ''; | ||
|
|
||
| position: absolute; | ||
| inset: calc(var(--eds-sizing-stroke-thick) * -2); |
There was a problem hiding this comment.
--eds-spacing-vertical-3xs * -1?
vnys
left a comment
There was a problem hiding this comment.
Stroke tokens used instead of spacing tokens, apart from that it LGTM 👍
|
@claude review |
|
Claude finished @pomfrida's task in 1m 53s —— View job Code Review: Link Component (Round 3)
The component has come a long way. All blocking issues from previous reviews have been addressed. Here's the current status: @vnys' CHANGES_REQUESTED — resolved ✅The latest commit (
The remaining uses of @millus' CHANGES_REQUESTED — mostly resolved
Two items still open1. There's no Suggested options:
2. Height discrepancy (Figma 15px vs browser 19px) @millus flagged that the Figma spec shows 15px total height for the standalone link, but the rendered component measures 19px. This is likely caused by the What looks good
The two open items above are design decisions rather than bugs — ready to merge once confirmed. |
millus
left a comment
There was a problem hiding this comment.
Looking good 🚀
A little something from claude to note down for future work:
For consumers using React Router or Next.js will likely want an as prop (or similar) to
swap out the underlying for a router link component. Not a
blocker at all, just worth tracking.
| render: () => ( | ||
| <Link href="#" variant="standalone"> | ||
| Standalone link | ||
| <Icon data={external_link} /> |
There was a problem hiding this comment.
Nitpick: I think it would be more clear to have a normal arrow or some other icon here to separate the story from the external link story, so its more visually different use cases
There was a problem hiding this comment.
Good point, but since we only have the external_link icon in Figma I think maybe this story is obsolete, so I'll just remove it (it's already covered in other stories)
- Replace hardcoded px values with design tokens for underline-offset, outline-offset and focus ring inset - Remove unnecessary cursor: pointer and duplicate display: inline-flex - Make href required in LinkProps - Make data-font-family conditional so inline inherits from context - Add data-line-height squished for standalone to match Figma height - Add ExternalLink story with target="_blank" pattern - Add tests for conditional data-font-family and data-line-height
…ot element" This reverts commit 3b64503.
…ink CSS Replace --eds-sizing-stroke-thick with proper spacing tokens (--eds-spacing-vertical-4xs, --eds-spacing-vertical-3xs) for text-underline-offset, outline-offset, and focus ring inset.
b11ee42 to
f910a73
Compare
Summary
next)inline(default) andstandalonevariants<Icon>as children)Test plan