feat(ds): add Link for navigation - #8179
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a typed Estimated code review effort: 3 (Moderate) | ~20 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Zaimwa9
left a comment
There was a problem hiding this comment.
Requesting changes for a quick one, sorry 🙏
2c86cb2 to
7d16a2a
Compare
Button renders an <a> whenever it is given href, across 30 call sites, so the name tells you nothing about what you get. btn-link also gets pasted onto bare <a> elements in a couple of places, and <Link><Button> nests a button inside an anchor, which is invalid HTML. Link does the other job. `to` renders a router link, `href` a plain anchor, and the type permits exactly one so a link cannot render without a destination. It owns its styling and imports nothing from Button. Link.scss reproduces what btn-link renders today so the call sites that move across later do not shift. --color-text-action is the same purple in light mode and a lighter one in dark, which btn-link had no treatment for. Six stories rather than a controls playground, so each one snapshots. The prose and icon ones earn their place: .link is inline-flex with a gap, so baseline alignment mid-sentence and icon spacing are what would break unnoticed. No adoption in this PR. Nothing uses it yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
React 19 passes ref as a normal prop, and Input and InputGroup already take it that way. Also drops the named export, since the folder barrel and every consumer use the default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
body.dark a in styles.scss colours every anchor at higher specificity than .link, so the token never applied and links rendered as body text. Dark is repeated to beat it, the same way .dark .btn-link already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
btn-link does have a dark treatment, .dark .btn-link keeps the same purple, so moving a link over is a visible change in dark mode rather than parity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A string cannot express router state. ReleaseManagerPage passes
state: { searchQuery } to the flag page, which reads it back via
location.state and promotes it into ?search= on the way back, so the object
form is load-bearing rather than stylistic.
Uses RouterLinkProps['to'] instead of restating the shape, so it cannot drift
from react-router. Both destinations at once, and neither, are still type
errors.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
target was destructured out and only re-applied on the anchor branch, so <Link to='/x' target='_blank'> silently opened in the same tab. rel had the same hole. Both branches now share one safeRel. Also extracts the destination union into a named LinkDestination, drops an empty comment line stylelint flags, and points the story types at @storybook/react-webpack5, which is where Storybook 10 keeps them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Truthiness meant an empty to fell through to the anchor branch and rendered an <a> with no href, and the type guarantees there is no href to fall back to. The router decides what '' resolves to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
btn-link now reads the same token, so dark no longer differs and .dark .btn-link is gone. The .dark .link selector stays: body.dark a is untouched and still outranks a bare .link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7d16a2a to
c59b6ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d12e3c80-84ce-4e81-bbba-36e233f54ef3
📒 Files selected for processing (1)
frontend/web/components/base/link/Link.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b25c3b5-935b-4aff-bb6a-db9206b26d8e
📒 Files selected for processing (4)
frontend/documentation/components/Link.stories.tsxfrontend/web/components/base/link/Link.scssfrontend/web/components/base/link/Link.tsxfrontend/web/components/base/link/index.ts
Zaimwa9
left a comment
There was a problem hiding this comment.
Approving as I think we can close our eyes on the relative import if you deem to
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8178 (epic #5746)
The goal is that in-app navigation goes through react-router's
Linkwherever it can, so the app does not reload, and that one component decides that rather than every call site choosing for itself. TodayButtonrenders an<a>whenever it is givenhref, which it is at 30 sites, so the name tells you nothing about what you get and each one is a full page load whether it needs to be or not.tois the default, rendering a router link.hrefis for leaving the app. The type permits exactly one, so a link cannot render without a destination and passing both is a compile error rather than one silently winning.Link.scssand imports nothing fromButton. In light mode it is the same#6837fcasbtn-link($link-colorresolves to$primary), so those call sites will not shift when they move over..dark .btn-linkkeeps that same purple; this takes--purple-400, which is lighter and holds up better on a dark background. Moving a link over is therefore a visible change in dark mode, not a no-op..linkrepeats itself under.dark.body.dark ainstyles.scsscolours every anchor at higher specificity than a bare.link, so the token never applied and links rendered as body text..dark .btn-linkalready exists for the same reason. Both disappear if that global rule moves to a token..linkisinline-flexwith agap, so baseline alignment mid-sentence and icon spacing are what would break unnoticed.Not in scope here: nothing uses it yet, deliberately, so this lands on its own. Adoption of the 30
Button hrefsites follows in batches under #8178, after whichButtoncan drop its anchor branch and only ever render<button>. Roughly 20 of those are external, a handful are internal routes that becometo, and about 6 arehref='#'with anonClick, which are not navigation and want a real button.How did you test this code?
tsc: 0 new errors vsmain.eslint: clean on all changed files.Components/Link.Nobody has looked at it rendered, hence draft. Smoke test (light + dark):
InSentencebeside abtn-linkelsewhere in the app: colour and weight match, baseline sits right mid-paragraphWithIcon:gap: 0.25remspacing between icon and textbtn-linkInApp: clicking navigates without a page reloadNewTab: opens a new tab andrel="noreferrer"is setAlongsideAButton: the link and the button read as different things