toggle and toggleGroup with color prop , test ,sb and docs #815
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
docs/app/docs/components/toggle-group/page.jsOops! Something went wrong! :( ESLint: 8.56.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/docs/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (7)
docs/app/docs/components/toggle/page.js (1)
21-23: Enhance documentation and uncomment relevant props.The documentation could be improved:
- The color prop description should include:
- Accepted color values/format
- Example usage
- The commented props (pressed, disabled) appear to be valid - consider documenting them if they're supported.
- {prop: 'color', type: 'string', default: 'null', description: 'Accent Color of the toggled item', id: 'color'}, + {prop: 'color', type: 'string', default: 'null', description: 'Sets the accent color of the toggled item. Accepts any valid CSS color value.', id: 'color'}, // {prop: 'pressed', type: 'boolean', default: 'false', description: 'Accent Color of the toggled item', id: 'pressed'}, // {prop: 'disabled', type: 'boolean', default: 'null', description: 'Accent Color of the toggled item', id: 'disabled'},docs/app/docs/components/toggle-group/page.js (1)
19-21: Maintain documentation consistency with Toggle component.The documentation should be consistent with the Toggle component:
- Enhance the color prop description
- Address the commented props if they're supported
- {prop: 'color', type: 'string', default: 'null', description: 'Accent Color of the toggled item', id: 'color'}, + {prop: 'color', type: 'string', default: 'null', description: 'Sets the accent color of the toggled items. Accepts any valid CSS color value.', id: 'color'}, // {prop: 'pressed', type: 'boolean', default: 'false', description: 'Accent Color of the toggled item', id: 'pressed'}, // {prop: 'disabled', type: 'boolean', default: 'null', description: 'Accent Color of the toggled item', id: 'disabled'},src/components/ui/Toggle/tests/Toggle.test.js (1)
50-53: Enhance color prop test coverage.While the basic test is good, consider adding more test cases:
- Test with different color values
- Test color prop updates
- Test default/fallback behavior when color is not provided
test('Toggle renders color correctly', () => { const { getByText } = render(<Toggle pressed={false} onChange={() => {}} color='blue'>Test Toggle</Toggle>); expect(getByText('Test Toggle')).toHaveAttribute('data-accent-color', 'blue'); }); + +test('Toggle handles color prop updates', () => { + const { getByText, rerender } = render(<Toggle pressed={false} onChange={() => {}} color='blue'>Test Toggle</Toggle>); + expect(getByText('Test Toggle')).toHaveAttribute('data-accent-color', 'blue'); + + rerender(<Toggle pressed={false} onChange={() => {}} color='red'>Test Toggle</Toggle>); + expect(getByText('Test Toggle')).toHaveAttribute('data-accent-color', 'red'); +}); + +test('Toggle without color prop', () => { + const { getByText } = render(<Toggle pressed={false} onChange={() => {}}>Test Toggle</Toggle>); + expect(getByText('Test Toggle')).not.toHaveAttribute('data-accent-color'); +});src/components/ui/Toggle/stories/Toggle.stories.js (1)
41-45: Enhance Color story implementation.Consider these improvements:
- Add consistent spacing after the colon
- Demonstrate multiple color variations using StoryBook controls
export const Color = { args: { - color:"blue" + color: "blue", + // Enable color control in StoryBook + argTypes: { + color: { + control: { type: 'select' }, + options: ['blue', 'red', 'green'] + } + } } }src/components/ui/ToggleGroup/tests/ToggleGroup.test.js (1)
93-99: Enhance ToggleGroup color test coverage.The current test verifies basic color functionality, but consider adding:
- Verification for all toggle items
- Test for color prop updates
- Test for color inheritance in multiple selection mode
test('ToggleGroup color correctly', () => { render(<ToggleGroup type="single" items={items} color='blue'/>); const toggleGroupRoot = document.querySelector('.rad-ui-toggle-group'); fireEvent.click(toggleGroupRoot.children[0]); expect(toggleGroupRoot.children[0]).toHaveAttribute('data-accent-color', 'blue'); + + // Verify all items have the color attribute + Array.from(toggleGroupRoot.children).forEach(child => { + expect(child).toHaveAttribute('data-accent-color', 'blue'); + }); }); + +test('ToggleGroup color updates propagate to children', () => { + const { rerender } = render(<ToggleGroup type="multiple" items={items} color='blue'/>); + const toggleGroupRoot = document.querySelector('.rad-ui-toggle-group'); + + fireEvent.click(toggleGroupRoot.children[0]); + fireEvent.click(toggleGroupRoot.children[1]); + + rerender(<ToggleGroup type="multiple" items={items} color='red'/>); + expect(toggleGroupRoot.children[0]).toHaveAttribute('data-accent-color', 'red'); + expect(toggleGroupRoot.children[1]).toHaveAttribute('data-accent-color', 'red'); +});src/components/ui/ToggleGroup/stories/ToggleGroup.stories.js (1)
72-79: Improve Color story implementation and formatting.The story implementation could be enhanced:
- Fix spacing after colon in color prop
- Add color controls for interactive documentation
- Consider demonstrating both single and multiple selection with colors
export const Color = { args: { className: '', type: 'multiple', - color:"blue", + color: "blue", items + }, + argTypes: { + color: { + control: { type: 'select' }, + options: ['blue', 'red', 'green'] + } } }styles/themes/components/toggle.scss (1)
18-19: Consider adding transition for smoother state changes.The darker background color (accent-900) with white text provides good contrast in the "on" state. However, the state change might feel abrupt.
Consider adding a transition for a smoother user experience:
.rad-ui-toggle { + transition: background-color 0.2s ease-in-out, color 0.2s ease-in-out; /* existing styles */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/app/docs/components/toggle-group/page.js(2 hunks)docs/app/docs/components/toggle/page.js(1 hunks)src/components/ui/Toggle/Toggle.tsx(4 hunks)src/components/ui/Toggle/stories/Toggle.stories.js(1 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/components/ui/ToggleGroup/ToggleGroup.tsx(1 hunks)src/components/ui/ToggleGroup/stories/ToggleGroup.stories.js(1 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(1 hunks)styles/themes/components/toggle.scss(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/ui/Toggle/Toggle.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🔇 Additional comments (6)
src/components/ui/ToggleGroup/ToggleGroup.tsx (2)
9-13: LGTM! Clean implementation of data attributes.The data attributes implementation is clean and type-safe using Record<string, string>.
19-21: LGTM! Proper spreading of data attributes.The data attributes are correctly spread to the ToggleItem component.
src/components/ui/Toggle/Toggle.tsx (3)
16-16: LGTM! Proper type definition for color prop.The color prop is correctly typed as an optional string in ToggleProps.
44-48: LGTM! Consistent implementation with ToggleGroup.The data attributes implementation matches the pattern used in ToggleGroup component.
58-60: LGTM! Correct props spreading order.The spreading order ensures data_attributes take precedence over any conflicting attributes from props.
styles/themes/components/toggle.scss (1)
18-19: LGTM! Good accessibility considerations.The changes maintain strong accessibility practices:
- High contrast between white text and accent-900 background
- Preserved focus visibility
- Use of semantic color variables
| const COMPONENT_NAME = 'ToggleGroup'; | ||
|
|
||
| const ToggleGroup = ({ type = 'single', items = [] as { value: any; label: any }[] }) => { | ||
| const ToggleGroup = ({ type = 'single', items = [] as { value: any; label: any }[], color=''}) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type definition for the color prop.
The color prop should have a proper type definition in the function parameters.
-const ToggleGroup = ({ type = 'single', items = [] as { value: any; label: any }[], color=''}) => {
+type ToggleGroupProps = {
+ type?: 'single' | 'multiple';
+ items?: Array<{ value: any; label: any }>;
+ color?: string;
+};
+
+const ToggleGroup = ({ type = 'single', items = [], color = '' }: ToggleGroupProps) => {📝 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 ToggleGroup = ({ type = 'single', items = [] as { value: any; label: any }[], color=''}) => { | |
| type ToggleGroupProps = { | |
| type?: 'single' | 'multiple'; | |
| items?: Array<{ value: any; label: any }>; | |
| color?: string; | |
| }; | |
| const ToggleGroup = ({ type = 'single', items = [], color = '' }: ToggleGroupProps) => { |
kotAPI
added
the
automerge
| items.map((item, index) => { | ||
| return ( | ||
| <ToggleItem key={index} value={item.value}> | ||
| <ToggleItem key={index} value={item.value} {...data_attributes}> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/ToggleGroup/ToggleGroup.tsx (1)
14-14: Avoid using array index as key.Using array indices as keys can lead to issues with component identity and reordering. Consider using a unique identifier from the item data:
-<ToggleItem key={index} value={item.value}> +<ToggleItem key={item.value} value={item.value}>If
item.valueis not guaranteed to be unique, consider adding anidfield to your items data structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/ToggleGroup/ToggleGroup.tsx(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/ToggleGroup/tests/ToggleGroup.test.js
🧰 Additional context used
🪛 eslint
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx
[error] 8-8: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/ToggleGroup/ToggleGroup.tsx
[error] 8-8: Operator '=' must be spaced.
(space-infix-ops)
[error] 8-8: A space is required before '}'.
(object-curly-spacing)
🔇 Additional comments (3)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (2)
46-50: LGTM! Clean implementation of data attributes.The implementation of data attributes is clean and type-safe using Record<string, string>.
53-53: LGTM! Good accessibility and implementation practices.The JSX implementation follows React best practices with proper accessibility attributes and clean class composition.
src/components/ui/ToggleGroup/ToggleGroup.tsx (1)
8-8: 🛠️ Refactor suggestionFix spacing issues and improve type definitions.
There are spacing issues around the equals operator and curly braces. Also, let's improve the type definitions:
-const ToggleGroup = ({ type = 'single', items = [] as { value: any; label: any }[], color=''}) => { +type ToggleItem = { + value: string; + label: React.ReactNode; +}; + +type ToggleGroupProps = { + type?: 'single' | 'multiple'; + items?: ToggleItem[]; + color?: string; +}; + +const ToggleGroup = ({ type = 'single', items = [], color = '' }: ToggleGroupProps) => {Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 8-8: Operator '=' must be spaced.
(space-infix-ops)
[error] 8-8: A space is required before '}'.
(object-curly-spacing)
| import { ToggleContext } from '../contexts/toggleContext'; | ||
|
|
||
| const ToggleGroupRoot = ({ type = 'multiple', className = '', loop = true, customRootClass = '', componentName = '', value = null, children }:any) => { | ||
| const ToggleGroupRoot = ({ type = 'multiple', className = '', loop = true, customRootClass = '', componentName = '', value = null, color='', children }:any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript type definitions for the component props.
The component is using :any type which defeats TypeScript's type safety. Let's add proper type definitions:
+type ToggleGroupRootProps = {
+ type?: 'single' | 'multiple';
+ className?: string;
+ loop?: boolean;
+ customRootClass?: string;
+ componentName?: string;
+ value?: string | string[] | null;
+ color?: string;
+ children: React.ReactNode;
+};
-const ToggleGroupRoot = ({ type = 'multiple', className = '', loop = true, customRootClass = '', componentName = '', value = null, color='', children }:any) => {
+const ToggleGroupRoot = ({
+ type = 'multiple',
+ className = '',
+ loop = true,
+ customRootClass = '',
+ componentName = '',
+ value = null,
+ color = '',
+ children
+}: ToggleGroupRootProps) => {📝 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 ToggleGroupRoot = ({ type = 'multiple', className = '', loop = true, customRootClass = '', componentName = '', value = null, color='', children }:any) => { | |
| type ToggleGroupRootProps = { | |
| type?: 'single' | 'multiple'; | |
| className?: string; | |
| loop?: boolean; | |
| customRootClass?: string; | |
| componentName?: string; | |
| value?: string | string[] | null; | |
| color?: string; | |
| children: React.ReactNode; | |
| }; | |
| const ToggleGroupRoot = ({ | |
| type = 'multiple', | |
| className = '', | |
| loop = true, | |
| customRootClass = '', | |
| componentName = '', | |
| value = null, | |
| color = '', | |
| children | |
| }: ToggleGroupRootProps) => { |
🧰 Tools
🪛 eslint
[error] 8-8: Operator '=' must be spaced.
(space-infix-ops)
| </Documentation.ComponentHero> | ||
|
|
||
|
|
||
| <div className="max-w-screen-md"> |
There was a problem hiding this comment.
these width classess should be global, and not just for this page
There was a problem hiding this comment.
Ok, so this should be in the documentation component?
| <ToggleWrapper/> | ||
| </Documentation.ComponentHero> | ||
|
|
||
| <div className="max-w-screen-md"> |
There was a problem hiding this comment.
whats the benefit of this class here?
|
|
||
| const DocsTable = ({ children, columns = [], data = [] }) => { | ||
| return <div className='mb-20'> | ||
| return <div className='mb-20 max-w-screen-md'> |
There was a problem hiding this comment.
Prevents stretch after a certain width like in avatar api doc
2 participants
Made the css uniform for toggle with toggle grp and commented out some api's for docs that can be used
Summary by CodeRabbit
New Features
colorprop to Toggle and ToggleGroup components.Documentation
Style Changes
onstate.Testing