Accordion Variants #787
Accordion Variants #787
Conversation
WalkthroughThis pull request introduces enhancements to the Accordion and Button components, focusing on adding customization options through new optional properties like Changes
Sequence DiagramsequenceDiagram
participant User
participant AccordionRoot
participant AccordionItem
participant AccordionHeader
participant AccordionTrigger
User->>AccordionRoot: Render with variant/color
AccordionRoot->>AccordionItem: Pass context
AccordionItem->>AccordionHeader: Render with expanded state
AccordionHeader->>AccordionTrigger: Apply styling
User->>AccordionTrigger: Interact with trigger
AccordionTrigger->>AccordionItem: Toggle expanded state
Possibly related PRs
Suggested labels
Poem
Finishing Touches
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: 2
🔭 Outside diff range comments (1)
src/components/ui/Button/stories/Button.stories.js (1)
Line range hint
1-86: Convert file to TypeScript.This file contains JSX and would benefit from TypeScript's type safety. Consider:
- Renaming to
.tsx- Adding proper types for props and components
- Using the Story type from Storybook
🧹 Nitpick comments (9)
styles/themes/components/accordion.scss (1)
7-30: Reduce code duplication using SCSS mixins.The accordion item styles are duplicated in the outline variant. Consider extracting common styles into a mixin for better maintainability.
@mixin accordion-item-base { width: 100%; display: flex; flex-direction: column; flex: 1; margin-bottom: 12px; border-radius: 10px; overflow: hidden; &:focus { outline: none; border: none; } &:focus-within { border: 1px solid var(--rad-ui-color-accent-900); color: var(--rad-ui-color-accent-900); } } .rad-ui-accordion-item { @include accordion-item-base; // unique styles for default variant } &[data-variant='outline'] .rad-ui-accordion-item { @include accordion-item-base; // unique styles for outline variant }src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)
11-12: Remove unused context values.The following context values are imported but never used in the component:
- From AccordionContext:
setActiveItem,focusNextItem,focusPrevItem- From AccordionItemContext:
handleBlurEvent,handleClickEvent,handleFocusEvent- const { setActiveItem, rootClass, focusNextItem, focusPrevItem, activeItem } = useContext(AccordionContext); - const { itemValue, handleBlurEvent, handleClickEvent, handleFocusEvent } = useContext(AccordionItemContext); + const { rootClass, activeItem } = useContext(AccordionContext); + const { itemValue } = useContext(AccordionItemContext);src/components/ui/Accordion/Accordion.tsx (1)
15-16: Consider using union types for better type safety.Instead of using string type for variant and color, consider defining specific allowed values using union types.
- variant?: string; - color?: string; + variant?: 'default' | 'bordered' | 'contained'; + color?: 'primary' | 'secondary' | 'accent';src/components/ui/Button/Button.tsx (2)
Line range hint
8-8: Remove or implement the TODO comment.The comment "make the color prop default accent color" appears to be a TODO item that hasn't been implemented.
Either implement the default color or remove the comment.
13-13: Consider using union types for better type safety.Similar to the Accordion component, consider using a union type for the color prop to restrict it to valid values.
- color?: string; + color?: 'primary' | 'secondary' | 'accent';src/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
12-14: Fix inconsistent type definitions.The type definitions for variant and color use an unusual union with empty string literal:
- variant?: string | '' - color?: string | '' + variant?: string + color?: stringConsider using union types for better type safety:
variant?: 'default' | 'bordered' | 'contained' color?: 'primary' | 'secondary' | 'accent'
33-33: Remove empty useEffect hook.The useEffect hook is empty and has no dependencies.
- useEffect(() => {}, []);src/components/ui/Accordion/stories/Accordion.stories.tsx (1)
6-47: Consider enhancing the movie data structure.The movie data structure is well-organized but could benefit from some improvements:
- The summary for "The Shawshank Redemption" appears incomplete
- Consider adding types for the movie data structure
interface MovieItem { title: string; content: React.ReactNode; } const items: MovieItem[] = [ // ... existing items ];src/components/ui/Button/stories/Button.stories.js (1)
69-86: Enhance the Color story implementation.The Color story implementation could be improved:
- ArrowIcon's color isn't properly handled for different button variants
- The story lacks TypeScript types
- Consider using a consistent layout with other stories
export const Color: Story = () => ( <SandboxEditor> <div className='space-y-4'> <div> <h3 className='text-gray-950 mb-2'>Button Colors</h3> <div className='flex items-center space-x-[40px]'> <Button color='red'> <span>{BUTTON_TEXT}</span> <ArrowIcon className={variant === 'solid' ? 'text-white' : 'text-accent-900'} /> </Button> <Button> <span>{BUTTON_TEXT}</span> <ArrowIcon className={variant === 'solid' ? 'text-white' : 'text-accent-900'} /> </Button> </div> </div> </div> </SandboxEditor> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/ui/Accordion/Accordion.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionHeader.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(3 hunks)src/components/ui/Accordion/fragments/AccordionTrigger.tsx(0 hunks)src/components/ui/Accordion/stories/Accordion.stories.tsx(2 hunks)src/components/ui/Button/Button.tsx(2 hunks)src/components/ui/Button/stories/Button.stories.js(2 hunks)src/components/ui/Code/tests/Code.test.tsx(1 hunks)src/components/ui/Em/tests/Em.test.js(1 hunks)src/components/ui/Table/tests/Table.test.tsx(1 hunks)styles/themes/components/accordion.scss(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/Accordion/fragments/AccordionTrigger.tsx
✅ Files skipped from review due to trivial changes (3)
- src/components/ui/Code/tests/Code.test.tsx
- src/components/ui/Table/tests/Table.test.tsx
- src/components/ui/Em/tests/Em.test.js
🔇 Additional comments (10)
styles/themes/components/accordion.scss (2)
1-4: LGTM! Root styles are clean and consistent.
56-59: LGTM! Content styles use design system colors appropriately.src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)
14-14: LGTM! Good use of data attributes for styling hooks.The expanded state is correctly derived from context values and properly exposed via data attributes for styling purposes.
Also applies to: 17-17
src/components/ui/Accordion/Accordion.tsx (1)
19-19: LGTM! Props are correctly passed down.The variant and color props are properly initialized with default values and passed to AccordionRoot.
Also applies to: 21-21
src/components/ui/Button/Button.tsx (1)
31-33: LGTM! Consistent use of data attributes.The data attribute handling for color follows the same pattern as other attributes, maintaining consistency.
src/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
24-31: LGTM! Good implementation of data attributes.The data attributes are properly constructed and applied to the root element, providing good styling hooks.
Also applies to: 72-72
38-38: LGTM! Proper implementation of loop functionality.The loop prop is correctly passed to the navigation helper functions.
Also applies to: 50-50
src/components/ui/Accordion/stories/Accordion.stories.tsx (2)
1-1: LGTM! Good type safety improvement.Adding the
AccordionPropstype import enhances type safety and documentation.
70-86: Verify story variants align with design system.The stories effectively demonstrate the new variant and color capabilities. However, ensure that:
- The 'solid' and 'outline' variants align with your design system
- The 'blue' color choice is part of your approved color palette
✅ Verification successful
Variants and colors are aligned with the design system
The 'solid' and 'outline' variants are properly integrated into the design system, as evidenced by their consistent implementation across components (Button, Accordion) and their inclusion in the standard variant set ('solid', 'soft', 'outline', 'ghost'). The 'blue' color is also validated as a standard accent color in the theming system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent variant/color usage across components rg -g '*.{tsx,scss}' -A 2 "variant.*=.*['\"](solid|outline)['\"]\|color.*=.*['\"]blue['\"]\|@include.*variant\((solid|outline)\)"Length of output: 139
Script:
#!/bin/bash # Search for variant usage rg -g '*.{tsx,scss}' "variant.*['\"]solid['\"]" rg -g '*.{tsx,scss}' "variant.*['\"]outline['\"]" # Search for color usage rg -g '*.{tsx,scss}' "color.*['\"]blue['\"]" # Look for design system files fd -e ts -e tsx -e scss "theme|tokens|colors|variants" --type f # Check content of any design system related files fd -e ts -e tsx -e scss "theme|tokens|colors|variants" --type f -x head -n 20 {}Length of output: 2414
src/components/ui/Button/stories/Button.stories.js (1)
Line range hint
40-44: Consider documenting the breaking change.Renaming the export from 'All' to 'AllVariants' is a breaking change that could affect existing story references.
| .rad-ui-accordion-content { | ||
| padding: 12px 16px; | ||
| color: var(--rad-ui-color-gray-950); | ||
| font-weight: 400; | ||
| font-size: 0.875rem; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent typography across variants.
Typography styles (font-weight and font-size) are only defined in the outline variant. This could lead to inconsistent text styling between variants. Consider moving these styles to the base content class.
.rad-ui-accordion-content {
background-color: var(--rad-ui-color-accent-50);
padding: 12px 16px;
color: var(--rad-ui-color-gray-950);
+ font-weight: 400;
+ font-size: 0.875rem;
}Committable suggestion skipped: line range outside the PR's diff.
| .rad-ui-accordion-header { | ||
|
|
||
| // and if data expanded is true, add bottom border | ||
| &[data-expanded="true"] { | ||
| border-bottom: 1px solid var(--rad-ui-color-accent-700); | ||
| color: var(--rad-ui-color-accent-900); | ||
|
|
||
| } | ||
|
|
||
| .rad-ui-accordion-trigger { | ||
| background-color: var(--rad-ui-color-accent-50); | ||
| padding: 12px 16px; | ||
| width: 100%; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
|
|
||
| &:focus { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
Address accessibility concerns with focus states.
-
Removing the focus outline without providing an alternative visual indicator could impact keyboard navigation accessibility. Consider adding a visible focus indicator.
-
The comment on line 35 is redundant as the code is self-documenting through the data attribute usage.
- // and if data expanded is true, add bottom border
&[data-expanded="true"] {📝 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.
| .rad-ui-accordion-header { | |
| // and if data expanded is true, add bottom border | |
| &[data-expanded="true"] { | |
| border-bottom: 1px solid var(--rad-ui-color-accent-700); | |
| color: var(--rad-ui-color-accent-900); | |
| } | |
| .rad-ui-accordion-trigger { | |
| background-color: var(--rad-ui-color-accent-50); | |
| padding: 12px 16px; | |
| width: 100%; | |
| display: flex; | |
| justify-content: space-between; | |
| align-items: center; | |
| &:focus { | |
| outline: none; | |
| } | |
| .rad-ui-accordion-header { | |
| &[data-expanded="true"] { | |
| border-bottom: 1px solid var(--rad-ui-color-accent-700); | |
| color: var(--rad-ui-color-accent-900); | |
| } | |
| .rad-ui-accordion-trigger { | |
| background-color: var(--rad-ui-color-accent-50); | |
| padding: 12px 16px; | |
| width: 100%; | |
| display: flex; | |
| justify-content: space-between; | |
| align-items: center; | |
| &:focus { | |
| outline: none; | |
| } |
Summary by CodeRabbit
New Features
Style Changes
Documentation
Chores