ENG-3461: add unstyled mode to RouterLink, replace all remaining NextLink usages#7946
ENG-3461: add unstyled mode to RouterLink, replace all remaining NextLink usages#7946gilluminate merged 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: ENG-3461 — RouterLink unstyled mode + NextLink migration
This is a clean, well-scoped refactor. The discriminated union type for RouterLinkProps is the right design for distinguishing styled vs. unstyled modes, and the test coverage for the new unstyled mode is thorough. The migration across 22 files is mechanical and consistent.
Summary of findings
Minor / Nit:
- Double
target/reldestructure inRouterLink.tsx— works correctly but can be simplified by moving both to the top-level destructure (see inline comment). - Button-child cast to
AnchorProps— technically unsound but harmless in practice; the three-mode doc comment is clear enough that this is unlikely to cause issues.
Worth verifying before merge:
regulations.tsx— the Chakracolor="complimentary.500"on the original link was dropped.Typography.Linkhas its own default color which may differ; confirm it still matches the design.useSystemFormTabs.tsx— the originaltextDecor="underline"was dropped. If always-underlined was intentional, the styling needs to be carried forward viaclassName.configure.tsx—hover:border-[var(--fidesui-complimentary-500)]is functional but references the CSS variable by its raw name; worth a comment to aid future maintainers.
Overall the architecture is solid and the unstyled mode fills the right gap. The visual-regression notes above are worth a quick design check before merging.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
Extend RouterLink with an `unstyled` prop that delegates to NextLink directly, rendering a plain <a> without Typography.Link styling. This covers cards, list items, nav links, and other non-text content. Also forward className in button mode, and migrate all 22 remaining files that imported next/link directly. RouterLink.tsx is now the sole file importing from next/link. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move target and rel into the top-level destructure, eliminating redundant re-extraction and eslint-disable comments in each rendering branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0bdd320 to
612abf8
Compare
lucanovera
left a comment
There was a problem hiding this comment.
Code changes look good. The links kept the same styling in all places. Approved!
Ticket ENG-3461
Description Of Changes
Followup to #7942. Extends
RouterLinkwith anunstyledprop and migrates all 22 remaining files that importednext/linkdirectly, soRouterLink.tsxis now the sole file importing fromnext/link.The
unstyledprop delegates toNextLinkdirectly, rendering a plain<a>without Typography.Link styling. This covers wrapping cards, list items, nav links, search results, breadcrumbs, and other non-text content that shouldn't inherit link colors/underlines.Also forwards
classNamein button mode (previously dropped).Code Changes
unstyledboolean prop with discriminated union types (AnchorPropswhen unstyled,TypographyLinkPropsotherwise). Both button mode and unstyled mode now delegate toNextLink. AddedclassNameforwarding in button mode.NextLink,Link from "next/link",<Link as={NextLink}>(Chakra), and<LinkOverlay as={NextLink}>(Chakra) patterns withRouterLinkTypographyandButtonto fidesui mock since NavSearchModal now imports RouterLinkLinkBox/LinkOverlaypattern withRouterLink unstyled+ Tailwind classesSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works