Visually Hidden Component #665
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Component as VisuallyHidden
participant Renderer as React Renderer
participant Styles as CSS Styles
participant Accessibility as Screen Reader
Component->>Renderer: Render with props
Renderer->>Styles: Apply visually hidden class
Renderer->>Accessibility: Maintain content accessibility
Accessibility-->>Renderer: Content readable
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx (3)
1-1: Remove unused importThe
Childrenimport from React is not used in this file.-import React, { Children } from 'react'; +import React from 'react';
9-13: Fix indentation in render functionThe indentation of the children prop is inconsistent.
render: (args: VisuallyHiddenProps) => <SandboxEditor> <VisuallyHidden {...args}> - {args.children} - </VisuallyHidden> + {args.children} + </VisuallyHidden> </SandboxEditor>
18-30: Add proper documentation for component propsThe stories should document the available props and their purpose.
Add the following above the Default story:
/** * Props: * - children: Content to be visually hidden * - className: Additional CSS classes * - customRootClass: Override default root class * - asChild: Render children directly instead of wrapping div */🧰 Tools
🪛 eslint
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
[error] 28-28: Unexpected trailing comma.
(comma-dangle)
styles/themes/default.scss (2)
26-26: Consider maintaining alphabetical import order.While the import addition is correct, consider moving it before
switch.scssto maintain alphabetical ordering with other component imports.@import "./components/separator.scss"; @import "./components/skeleton.scss"; +@import "./components/visuallyhidden.scss"; @import "./components/switch.scss"; -@import "./components/visuallyhidden.scss";
Line range hint
57-61: Remove empty CSS rules.The
.rad-ui-tooltiprule is empty and can be removed. If it's intended for future use, consider adding a TODO comment or implementing it now.-.rad-ui-tooltip{ - -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/VisuallyHidden/VisuallyHidden.tsx(1 hunks)src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx(1 hunks)src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js(1 hunks)styles/themes/components/visuallyhidden.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- styles/themes/components/visuallyhidden.scss
🧰 Additional context used
🪛 eslint
src/components/ui/VisuallyHidden/VisuallyHidden.tsx
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
[error] 18-20: Block must not be padded by blank lines.
(padded-blocks)
[error] 23-23: Newline required at end of file but not found.
(eol-last)
src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js
[error] 5-6: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 19-19: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 24-24: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 26-26: Newline required at end of file but not found.
(eol-last)
src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx
[error] 16-17: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
[error] 28-28: Unexpected trailing comma.
(comma-dangle)
🪛 GitHub Check: lint
src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js
[failure] 18-18:
Expected an assignment or function call and instead saw an expression
[failure] 23-23:
Expected an assignment or function call and instead saw an expression
🔇 Additional comments (3)
src/components/ui/VisuallyHidden/tests/VisuallyHidden.test.js (1)
1-1: Verify CSS implementation for visual hiding
The component relies on the rad-ui-visually-hidden class, but the SCSS implementation isn't included in the review.
styles/themes/default.scss (2)
26-26: Verify visuallyhidden.scss implementation.
Let's ensure the imported file exists and contains appropriate visually-hidden styles that maintain accessibility.
Line range hint 31-36: Review commented-out focus styles for accessibility impact.
The commented-out focus styles might be important for accessibility. Since this PR introduces an accessibility-focused component, we should ensure that proper focus management is in place.
Let's check if focus styles are defined elsewhere:
✅ Verification successful
Focus styles are properly implemented in component-specific files
The commented-out focus styles in default.scss can safely remain commented as each component has its own focus management implemented using modern :focus-visible or :focus selectors with appropriate outlines:
- Button: Uses
:focus-visiblewith 2px solid accent-900 - Toggle & Toggle Group: Uses
:focus-visiblewith 2px solid accent-900 - Textarea: Uses
:focuswith 2px solid accent-500 - Radio: Uses
:focuswith 2px solid accent-900 - Accordion: Intentionally removes outline but uses
:focus-withinfor container focus
The component-specific implementations provide better focus management than the generic styles that were commented out, and they follow modern accessibility practices by using :focus-visible where appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for focus-related styles in other files
rg -g '*.{scss,css}' '(:focus|outline:|focus-visible)' --type css
Length of output: 1632
| const VisuallyHidden = ({ children, customRootClass, className, ...props }: VisuallyHiddenProps) => { | ||
|
|
||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
| return <Primitive.div className={clsx(rootClass, className)} {...props}>{children}</Primitive.div> | ||
|
|
There was a problem hiding this comment.
Add proper CSS class for visual hiding and ARIA attributes
The component is missing:
- The actual CSS class that handles visual hiding
- Proper ARIA attributes for accessibility
Apply this fix:
const VisuallyHidden = ({ children, customRootClass, className, ...props }: VisuallyHiddenProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
- return <Primitive.div className={clsx(rootClass, className)} {...props}>{children}</Primitive.div>
+ return (
+ <Primitive.div
+ className={clsx(rootClass, 'rad-ui-visually-hidden', className)}
+ aria-hidden="false"
+ {...props}
+ >
+ {children}
+ </Primitive.div>
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const VisuallyHidden = ({ children, customRootClass, className, ...props }: VisuallyHiddenProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| return <Primitive.div className={clsx(rootClass, className)} {...props}>{children}</Primitive.div> | |
| const VisuallyHidden = ({ children, customRootClass, className, ...props }: VisuallyHiddenProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| return ( | |
| <Primitive.div | |
| className={clsx(rootClass, 'rad-ui-visually-hidden', className)} | |
| aria-hidden="false" | |
| {...props} | |
| > | |
| {children} | |
| </Primitive.div> | |
| ); | |
| } |
🧰 Tools
🪛 eslint
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
| describe('VisuallyHidden Component', () => { | ||
| test('renders VisuallyHidden component', () => { | ||
| render(<VisuallyHidden>Visually Hidden</VisuallyHidden>); | ||
| expect(screen.getByText('Visually Hidden')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('renders VisuallyHidden component with className', () => { | ||
| render(<VisuallyHidden className='extra-classname' >Visually Hidden</VisuallyHidden>); | ||
| expect(screen.getByText('Visually Hidden')).toHaveClass('extra-classname'); | ||
| }); | ||
|
|
||
| test('renders VisuallyHidden component without asChild prop', () => { | ||
| render(<VisuallyHidden >Visually Hidden</VisuallyHidden>); | ||
| expect(screen.getByText('Visually Hidden')).tagName === 'DIV'; | ||
| }); | ||
|
|
||
| test('renders VisuallyHidden component with asChild prop as span', () => { | ||
| render(<VisuallyHidden asChild><span>Visually Hidden</span></VisuallyHidden>); | ||
| expect(screen.getByText('Visually Hidden')).tagName === 'SPAN'; | ||
| }); | ||
| }) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing test cases
The test suite should cover:
- customRootClass prop functionality
- Presence of rad-ui-visually-hidden class
- ARIA attributes
Add these test cases:
test('renders with custom root class', () => {
render(<VisuallyHidden customRootClass="custom-root">Content</VisuallyHidden>);
expect(screen.getByText('Content')).toHaveClass('custom-root');
});
test('applies visually-hidden class', () => {
render(<VisuallyHidden>Content</VisuallyHidden>);
expect(screen.getByText('Content')).toHaveClass('rad-ui-visually-hidden');
});
test('has correct aria attributes', () => {
render(<VisuallyHidden>Content</VisuallyHidden>);
expect(screen.getByText('Content')).toHaveAttribute('aria-hidden', 'false');
});🧰 Tools
🪛 eslint
[error] 19-19: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 24-24: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 26-26: Newline required at end of file but not found.
(eol-last)
🪛 GitHub Check: lint
[failure] 18-18:
Expected an assignment or function call and instead saw an expression
[failure] 23-23:
Expected an assignment or function call and instead saw an expression
kotAPI
added
the
automerge
2 participants
#220. Visually hidden component with support for asChild as renders as div as default in DOM.
Summary by CodeRabbit
New Features
VisuallyHiddencomponent for rendering content that is visually hidden but accessible to screen readers..rad-ui-visually-hiddenfor visually hiding elements while maintaining accessibility.Documentation
VisuallyHiddencomponent to demonstrate its usage and variations.Tests
VisuallyHiddencomponent under various conditions.Chores