UI: Underline links to avoid using only color#33031
Conversation
|
View your CI Pipeline Execution ↗ for commit 70f956b
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThree typography component files are updated to change anchor link styling from no underline to underlined text. The modifications consistently update the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/components/components/typography/link/link.tsx (2)
134-144: Consider removing redundant textDecoration in nochrome hover state.The
textDecoration: 'underline'at line 141 is now redundant since the base style (line 70) already applies underline to all links. This can be simplified for clarity, though it doesn't cause any functional issues.Apply this diff if you'd like to simplify:
({ nochrome }) => nochrome ? { color: 'inherit', - '&:hover, &:active': { - color: 'inherit', - textDecoration: 'underline', - }, } : {},
66-87: Behavioral change verified as intentional and accessibility-positive.The base
textDecoration: 'underline'(line 70) now applies to all links, includingnochromevariants. Sincenochromeusescolor: 'inherit', the underline is necessary for accessibility—without it,nochromelinks would be indistinguishable from surrounding text. The change is sound.Minor optimization: The
textDecoration: 'underline'in thenochromevariant's hover/active state (lines 139–140) is now redundant and could be removed since it's already set in the base styles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/components/components/typography/DocumentWrapper.tsx(1 hunks)code/core/src/components/components/typography/elements/A.tsx(1 hunks)code/core/src/components/components/typography/link/link.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/components/components/typography/link/link.tsxcode/core/src/components/components/typography/elements/A.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/components/components/typography/link/link.tsxcode/core/src/components/components/typography/elements/A.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/components/components/typography/link/link.tsxcode/core/src/components/components/typography/elements/A.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/link/link.tsxcode/core/src/components/components/typography/elements/A.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/typography/elements/A.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/src/components/components/typography/DocumentWrapper.tsx (1)
65-68: LGTM! Accessibility improvement correctly applied.The underline styling ensures links are distinguishable without relying solely on color, addressing the "link-in-text-block" Axe rule violation. The existing special handling for heading anchors (line 93) remains unaffected.
code/core/src/components/components/typography/elements/A.tsx (1)
6-12: LGTM! Consistent accessibility improvement.The underline styling is correctly applied to the A component, maintaining consistency with the changes in DocumentWrapper.tsx and link.tsx.
|
I have some ideas around this formulating. See the conversation in #33139 if you are curious. |
|
Setting this aside as:
|
|
Closing this in favour of a better contributor PR |
Closes #33017
What I did
Fixes some violations of https://dequeuniversity.com/rules/axe/4.10/link-in-text-block?application=axeAPI.
Flagged on https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=6913769e99ba81e2674e55f8.
@MichaelArestad you may want to go in another direction (font-style, font-weight, box-shadow, border, etc.). Do let me know if you prefer another approach to fixing this.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit