A11y: Underline MDX links for WCAG SC 1.4.1 compliance#33139
Conversation
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for giving this a go, @NikhilChowdhury27
Please, when using AI, perform your own manual review before requesting a review from maintainers; there are many PRs and issues to review and the maintainers shouldn't need to delay other PRs to catch obvious issues with AI-generated code.
On the screenshots you provided, we can see links that are not underlined. Therefore, some CSS changes are missing. Dosubot suggested that sbdocs classes need to be updated and this is probably what's missing.
Sidnioulz
changed the title
|
Also, please make sure your AI does not remove any section from the PR template. We need the maintainer checklist and canary section preserved. |
|
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:
📝 WalkthroughWalkthroughDefault link styling changed from non-underlined to underlined (with thickness and offset) across core typography and docs; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/typography/DocumentWrapper.test.tsx (1)
19-87: Comprehensive accessibility test coverage!The test suite thoroughly validates underline styling across:
- Light and dark themes
- Single and multiple link scenarios
- Multiple paragraphs/blocks
The use of
getComputedStylewithtoContain('underline')is the correct approach, astextDecorationcan include additional values like color and style.Optional enhancement: Consider adding a test case for anchor links (e.g.,
<a className="anchor">) to verify they maintain the special styling defined at DocumentWrapper.tsx line 93, which removes underlines on hover for heading position markers. This would document the intentional exception to the underline rule.it('should not underline anchor position markers on hover', () => { const { container } = render( <ThemedDocumentWrapper> <h2> <a className="anchor" href="#heading"> Heading </a> </h2> </ThemedDocumentWrapper> ); const anchor = container.querySelector('a.anchor'); expect(anchor).toBeTruthy(); // Anchor links should still have underline by default const styles = window.getComputedStyle(anchor!); expect(styles.textDecoration).toContain('underline'); // Note: Testing :hover pseudo-class would require additional setup // This test documents the default state });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/components/components/typography/DocumentWrapper.test.tsx(1 hunks)code/core/src/components/components/typography/DocumentWrapper.tsx(1 hunks)code/core/src/components/components/typography/elements/A.tsx(1 hunks)
🧰 Additional context used
🧠 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.test.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/typography/DocumentWrapper.test.tsx (1)
code/core/src/components/components/typography/DocumentWrapper.tsx (1)
DocumentWrapper(3-310)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (3)
code/core/src/components/components/typography/DocumentWrapper.tsx (1)
67-67: Excellent accessibility improvement!This change ensures links are distinguishable by more than color alone, meeting WCAG 2.1 Level A (SC 1.4.1). The implementation correctly preserves the intentional
textDecoration: 'none'fora.anchorelements on hover (line 93), which are heading position markers rather than content links.code/core/src/components/components/typography/elements/A.tsx (1)
11-11: LGTM! Consistent accessibility enhancement.This change aligns perfectly with the DocumentWrapper.tsx update, ensuring that the A component also provides underlined links for improved accessibility.
code/core/src/components/components/typography/DocumentWrapper.test.tsx (1)
1-17: Well-structured test setup!The test file setup is clean with appropriate environment configuration and a reusable
ThemedDocumentWrapperhelper component. The use of happy-dom is suitable for testing DOM styles.
NikhilChowdhury27
force-pushed
the
fix-issue-33017-mdx-link-underlines
branch
2 times, most recently
from
78e7ddf to
9085e51
Compare
|
@Sidnioulz This is my first time contributing to Storybook and I definitely made some rookie mistakes with the screenshots. I've cleaned everything up now with all 3 CSS changes, proper tests. I really appreciate your patience and detailed feedback - once this gets merged I'll have way better context on how things work here and I promise I won't make the same mistakes again. Thanks for taking the time to guide me through this! 🙏 |
NikhilChowdhury27
force-pushed
the
fix-issue-33017-mdx-link-underlines
branch
from
b0610f2 to
f5f627b
Compare
|
Howdy! I appreciate the PR. We are still defining link styles and trying to find out exactly what is needed. I do expect to add underlines to link styles in MDX, but those underlines probably shouldn't be applied in instances where there an icon accompanying the link text. Conversely, we could remove the chevron from the Configure page. As far as the link styles, I am leaning toward something like this: text-decoration-thickness: 0.5px; /* may also want relative */
text-underline-offset: 0.11em; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/addons/docs/src/blocks/components/DocsPage.tsx (1)
107-132: Docs page link reset correctly underlines links while excluding.anchorThe
[toGlobalSelector('a')]block now underlines links by default, and the nested&.anchorrule removes the underline for anchor icons, matching the intended a11y behavior and keeping heading anchors visually clean.There is some inevitable duplication of link style (font size, line height, color, underline) with
A.tsxandDocumentWrapper.tsx, so if you later tweak underline thickness/offset you’ll need to touch all three. If link styling continues to evolve, consider extracting a shared “docs link” style helper to keep these in sync.code/core/src/components/components/typography/DocumentWrapper.test.tsx (1)
1-83: Accessibility underline tests forDocumentWrapperare solidThe tests cover light theme, dark theme, and multiple links, and asserting
getComputedStyle(...).textDecorationcontains'underline'is a pragmatic way to verify the styling without overfitting to exact CSS serialization.If you want a tiny cleanup, you can simplify the cleanup hook:
- afterEach(() => { - cleanup(); - }); + afterEach(cleanup);Otherwise this file looks good as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/addons/docs/src/blocks/components/DocsPage.test.tsx(1 hunks)code/addons/docs/src/blocks/components/DocsPage.tsx(2 hunks)code/core/src/components/components/typography/DocumentWrapper.test.tsx(1 hunks)code/core/src/components/components/typography/DocumentWrapper.tsx(2 hunks)code/core/src/components/components/typography/elements/A.test.tsx(1 hunks)code/core/src/components/components/typography/elements/A.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- code/addons/docs/src/blocks/components/DocsPage.test.tsx
- code/core/src/components/components/typography/elements/A.test.tsx
- code/core/src/components/components/typography/DocumentWrapper.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/typography/elements/A.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/typography/DocumentWrapper.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/typography/elements/A.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/typography/DocumentWrapper.test.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/typography/elements/A.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/typography/DocumentWrapper.test.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/elements/A.tsxcode/addons/docs/src/blocks/components/DocsPage.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Write meaningful unit tests that import and call functions being tested, not just verify syntax patterns
Use 'yarn vitest run --coverage ' to run tests with coverage reports and aim for 75%+ coverage of statements/lines
Focus test coverage on all branches, conditions, edge cases, error paths, and different input variations
Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
🧠 Learnings (9)
📓 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.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.tsx
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/components/components/typography/DocumentWrapper.test.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). (1)
- GitHub Check: normal
🔇 Additional comments (1)
code/core/src/components/components/typography/elements/A.tsx (1)
6-26: Default underline for links with explicit.anchoropt‑out looks goodSetting
textDecoration: 'underline'on the baseAcomponent while explicitly resetting.anchorto'none'cleanly implements “underline by default, but not on heading anchor icons.” This stays scoped to typography links and aligns with the docs/MDX behavior introduced elsewhere in the PR.No functional issues from this change on its own.
|
Hey Michael! Alternative approach: |
@NikhilChowdhury27 This is a great call and will also reduce conflicts with buttons as it is in unlikely they are in those contexts. Let's try this! |
NikhilChowdhury27
force-pushed
the
fix-issue-33017-mdx-link-underlines
branch
from
f44c2f1 to
db07347
Compare
|
Hey @MichaelArestad , Thanks for the feedback! I've updated the implementation based on your suggestions. What changed:
Technical approach:
Note: The "What's new?" page loads content from an external iframe ( I'll add two screenshots showing the fixed About page for reference. All tests pass ✅ Let me know if you'd like any adjustments! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/DocsPage.tsx (1)
117-120: DocsContent link underline behavior matches intent; confirm selector scopeThe underline + thickness/offset on
[toGlobalSelector('a')]combined with&.anchor { textDecoration: 'none' }mirrorsDocumentWrapperand should get MDX text links over the SC 1.4.1 bar while keeping structural anchors unstyled.Because
toGlobalSelector('a')targets all non-.sb-unstyledanchors, this will also catch any remaining anchors in docs that aren’t inside unstyled blocks. If there are button-like anchors still living outside.sb-unstyled, they’ll now be underlined. It’s probably fine, but worth a quick scan of key docs pages to ensure there are no unexpected underlines on UI-ish anchors; if you spot any, you could either wrap them insb-unstyledor narrow the selector further (e.g., to paragraph/list/table contexts).Also applies to: 133-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/addons/docs/src/blocks/components/DocsPage.test.tsx(1 hunks)code/addons/docs/src/blocks/components/DocsPage.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(1 hunks)code/core/src/components/components/typography/DocumentWrapper.test.tsx(1 hunks)code/core/src/components/components/typography/DocumentWrapper.tsx(2 hunks)code/core/src/components/components/typography/elements/A.test.tsx(1 hunks)code/core/src/components/components/typography/elements/A.tsx(2 hunks)code/core/src/components/components/typography/link/link.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- code/addons/docs/src/blocks/components/DocsPage.test.tsx
- code/core/src/components/components/typography/DocumentWrapper.test.tsx
- code/core/src/components/components/typography/elements/A.tsx
- code/core/src/components/components/typography/elements/A.test.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/typography/link/link.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/typography/link/link.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/typography/link/link.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/link/link.tsxcode/addons/docs/src/blocks/components/DocsPage.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
🧠 Learnings (3)
📓 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/link/link.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/typography/DocumentWrapper.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/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). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/components/components/Button/Button.tsx (1)
319-320: Good safeguard to keep button-styled anchors non-underlinedExplicitly forcing
textDecoration: 'none'onStyledButtonis a nice defensive addition now that docs/global styles underlineaelements. This will prevent<Button as="a">and similar button-styled controls from accidentally looking like text links.code/core/src/components/components/typography/DocumentWrapper.tsx (1)
67-70: Docs link underlines and anchor exception look consistentUnderlining
awith controlled thickness/offset while explicitly removing decoration ona.anchorfits the SC 1.4.1 goal and preserves anchors as structural markers only. The combination with the existing&:hover a.anchorrule keeps anchors clean in all states. No issues from my side.Also applies to: 84-85
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/components/components/typography/link/link.tsx (2)
66-73: Default underline + thickness/offset look good for SC 1.4.1The new base underline (with
textDecorationThickness: '0.5px'andtextUnderlineOffset: '0.11em') is consistent with the stated a11y goal and keeps the styling localized to links. Now that every link is underlined by default, thetextDecoration: 'underline'inside thenochromehover/active block (Line 144) has become redundant; you can optionally drop that property for clarity in a follow-up.
171-181: Ensure button-styled links never get underlines, even when combined with other variants
textDecoration: 'none'on the base selector correctly removes the default underline forisButtonlinks. However, if a caller ever combinesisButtonwith another variant that setstextDecorationon:hover/:active(e.g.,nochrome), those pseudo-classes can still reintroduce underlines.To make button-like links robustly non-underlined in all states, you could also override the pseudo-classes in the
isButtonbranch:({ isButton }) => isButton ? { border: 0, borderRadius: 0, background: 'none', padding: 0, fontSize: 'inherit', - // Button-styled links should not have underlines - textDecoration: 'none', + // Button-styled links should not have underlines in any state + textDecoration: 'none', + '&:hover, &:focus, &:active': { + textDecoration: 'none', + }, } : {}This keeps button variants visually consistent even if future style functions add underline rules on hover/active.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/typography/link/link.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/typography/link/link.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/link/link.tsx
🧠 Learnings (4)
📓 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/link/link.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/components/components/typography/link/link.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/typography/link/link.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). (1)
- GitHub Check: normal
|
View your CI Pipeline Execution ↗ for commit 7699d6f
☁️ Nx Cloud last updated this comment at |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Code LGTM. Let's clean up comments before merging though.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/typography/elements/A.tsx (1)
11-14: WCAG-linked underline styles look solid; optional: centralize shared underline tokensThe inline WCAG F73 reference plus
textDecoration/textDecorationThickness/textUnderlineOffsetcombo, along with the.anchorexception, clearly express the intended a11y behavior and match the PR goal. If these exact underline values are reused inLink/DocumentWrapper, consider extracting them into a shared constant or helper to keep future tweaks in sync across typography components.Also applies to: 27-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/docs/src/blocks/components/DocsPage.tsx(2 hunks)code/core/src/components/components/typography/elements/A.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/docs/src/blocks/components/DocsPage.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/components/components/typography/elements/A.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/components/components/typography/elements/A.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/elements/A.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/components/components/typography/elements/A.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/components/components/typography/elements/A.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.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). (1)
- GitHub Check: normal
|
@Sidnioulz Thanks for your feedbacks, I have resolved, please see if we can merge this pr. Also going forward I will keep in mind all the comments you have given |
6 participants
Closes #33017
What I did
Added
textDecoration: 'underline'to links in MDX/docs for WCAG 2.1 Level A accessibility compliance (Success Criterion 1.4.1 - Use of Color).Files changed:
code/addons/docs/src/blocks/components/DocsPage.tsx- Added underline to links in docs pages (.sbdocs-content)code/core/src/components/components/typography/DocumentWrapper.tsx- Added underline to links in MDX wrappercode/core/src/components/components/typography/elements/A.tsx- Added underline to anchor elementsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Tested:
DocsPage.test.tsx,DocumentWrapper.test.tsx)http://localhost:6006.anchor) correctly remain without underlinesDocumentation
Note: No documentation updates needed - visual accessibility enhancement only.
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
🦋 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>Visual Verification
All links now have underlines in both light and dark themes:
![Links with underlines - light theme]
![Links with underlines - dark theme]
References:
Summary by CodeRabbit
Style
Tests