[WEB-3926] feat: port underlying Accordion to radix, add product icons, menu styles, bump to 14.8.0 #537
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several updates primarily focused on the Accordion component in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 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 (
|
jamiehenson
force-pushed
the
radix-accordion
branch
from
7f24c8e to
e99f5f3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/core/Accordion/utils.ts (1)
52-56: Enhance type safety and reduce duplicationThe utility functions duplicate theme arrays. Consider using constants and leveraging TypeScript for better type safety.
+const TRANSPARENT_THEMES = ['transparent', 'darkTransparent'] as const; +const STATIC_THEMES = ['static', 'darkStatic'] as const; + export const isNonTransparentTheme = (theme: AccordionTheme) => - !["transparent", "darkTransparent"].includes(theme); + !TRANSPARENT_THEMES.includes(theme); export const isStaticTheme = (theme: AccordionTheme) => - ["static", "darkStatic"].includes(theme); + STATIC_THEMES.includes(theme);src/core/Icon/Icon.stories.tsx (2)
72-74: Consider adding documentation for the encapsulated icon variant.The story implementation looks good, but it would be helpful to add documentation parameters explaining the purpose and usage of encapsulated icons.
Add documentation like this:
export const DarkEncapsulatedIcons = { render: () => renderIcons([...iconNames.product], true), + parameters: { + docs: { + description: { + story: "Encapsulated icons provide a consistent background container for product icons in dark theme.", + }, + }, + }, };
76-78: Consider expanding icon set coverage for theme testing.The implementation looks good, but consider including other icon sets beyond just product icons to ensure consistent appearance across themes.
Consider this enhancement:
export const LightEncapsulatedIcons = { - render: () => renderIcons([...iconNames.product], true, "light"), + render: () => renderIcons([...iconNames.product, ...iconNames.gui], true, "light"), + parameters: { + docs: { + description: { + story: "Encapsulated icons with light theme, showing both product and GUI icons for comprehensive theme testing.", + }, + }, + }, };src/core/Accordion/Accordion.stories.tsx (1)
232-238: Add documentation and consider hover state implementationWhile the story demonstrates the headerCSS feature, it would benefit from:
- Adding documentation parameters to explain the purpose and usage of headerCSS customization
- Avoiding the use of
!importantin the hover stateConsider updating the story like this:
export const WithCustomHeaderCSS = { render: () => AccordionPresentation({ data, - options: { headerCSS: "bg-pink-400 hover:!bg-pink-600 h-40" }, + options: { headerCSS: "bg-pink-400 hover:bg-pink-600 h-40" }, }), + parameters: { + docs: { + description: { + story: + "Allows custom CSS classes to be applied to accordion headers. Use `headerCSS` in options to customize header styling.", + }, + }, + }, };tailwind.config.js (1)
318-325: LGTM! Well-structured keyframes for accordion animations.The keyframes are correctly defined to work with Radix UI's accordion content height variable, ensuring smooth height transitions for both expanding and collapsing states.
Note: These keyframes rely on Radix UI's
--radix-accordion-content-heightCSS variable, which is automatically managed by Radix. This is the recommended approach for implementing accordion animations with Radix UI.src/core/Accordion.tsx (1)
66-66: Simplify thetextClassassignment for clarityConsider using a ternary operator for better readability when assigning
textClass.Refactored code:
-const textClass = ((selectable && selectableText) || text) as ColorClass; +const textClass = (selectable ? selectableText : text) as ColorClass;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
src/core/icons/icon-gui-chevron-down.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-chevron-up.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-hand.svgis excluded by!**/*.svgsrc/core/icons/icon-product-asset-tracking-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-chat-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-livesync-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-platform-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-pub-sub-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-spaces-mono.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(2 hunks)src/core/Accordion.tsx(2 hunks)src/core/Accordion/Accordion.stories.tsx(2 hunks)src/core/Accordion/types.ts(3 hunks)src/core/Accordion/utils.ts(1 hunks)src/core/Icon/Icon.stories.tsx(1 hunks)src/core/styles/colors/types.ts(1 hunks)tailwind.config.js(1 hunks)
🔇 Additional comments (16)
src/core/Accordion/types.ts (2)
3-3: LGTM! Good enhancement of color customization.
The change from ColorClass to DataStateOpenColorClass for selectable states aligns well with Radix UI's data-state pattern, allowing for different colors based on the accordion's open/closed state.
Also applies to: 39-40
50-51: LGTM! Enhanced customization options.
The new optional properties provide good flexibility:
headerCSSallows for custom header stylinghideBordersprovides control over border visibility
These additions improve the component's adaptability to different design requirements.
Let's verify the usage of these new options:
✅ Verification successful
Let me check the implementation details and stories to verify how these new properties are being used.
Let me check the actual component implementation since we couldn't find the Accordion.tsx file in the expected location.
Both new properties are properly implemented and demonstrated
The verification confirms that:
headerCSSis correctly applied to the AccordionTrigger component using clsx conditional classeshideBordersis properly implemented in the border class condition- The properties are demonstrated in stories (WithCustomHeaderCSS example)
The implementation is clean and follows the component's patterns:
headerCSSallows custom styling through string class nameshideBorderstoggles border visibility through a boolean flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new options are being used in the codebase
# Test 1: Check implementation of headerCSS
ast-grep --pattern 'headerCSS'
# Test 2: Check implementation of hideBorders
ast-grep --pattern 'hideBorders'
# Test 3: Check for stories using these options
rg -l "headerCSS|hideBorders" --glob "*.stories.tsx"
Length of output: 159
Script:
#!/bin/bash
# Check the implementation in Accordion component
rg -A 5 "headerCSS|hideBorders" src/core/Accordion/Accordion.tsx
# Check how these props are used in stories
cat src/core/Accordion/Accordion.stories.tsx
Length of output: 6633
Script:
#!/bin/bash
# Find the Accordion component file
fd Accordion.tsx
# Once found, let's check its content
rg -A 5 "headerCSS|hideBorders" --type tsx
Length of output: 114
Script:
#!/bin/bash
# Check the implementation in the found Accordion component file
cat src/core/Accordion.tsx
Length of output: 4328
src/core/Accordion/utils.ts (2)
1-56: Verify theme accessibility and integration
Please ensure that:
- All theme combinations meet WCAG color contrast requirements
- The Radix UI integration properly applies these theme classes
#!/bin/bash
# Check theme usage in Accordion component
echo "Checking theme usage in Accordion component..."
rg -A 5 "themeClasses|isNonTransparentTheme|isStaticTheme" --type tsx
# Look for existing accessibility tests
echo "Checking for accessibility tests..."
rg -l "describe|test|it.*accessibility" --type ts --type tsx | grep -i "accordion"38-39:
Fix invisible toggle icons in static themes
The toggle icons in static themes are set to the same color as their background (text-neutral-200 and text-neutral-1200), making them invisible. This appears to be intentional but could cause accessibility issues.
Consider using a visible color or adding aria-hidden="true" to the toggle icon if it's meant to be invisible.
Also applies to: 46-47
src/core/styles/colors/types.ts (1)
39-40: Well-structured type definition for Radix UI integration!
The new DataStateOpenColorClass type effectively extends the existing color system to support Radix UI's data attribute pattern for styling open states. This is a clean implementation that maintains type safety while enabling the Accordion component's theme configuration.
package.json (2)
3-3: LGTM! Version bump follows semver.
The version increment from 14.7.8 to 14.8.0 correctly follows semantic versioning for a new feature (Radix Accordion).
77-77: LGTM! Dependencies align with Radix migration.
The new dependencies are appropriate for the Accordion refactor:
@radix-ui/react-accordion: Required for the Radix UI portclsx: Standard utility for managing conditional class names
Both packages are using stable versions with proper semver ranges.
Also applies to: 80-80
src/core/Accordion/Accordion.stories.tsx (2)
60-64: LGTM: Theme-based text color enhancement
The conditional text color application based on the theme improves visibility and maintains consistency with the container's background color logic.
232-238: Verify headerCSS compatibility with Radix
Since this is part of the Radix migration, let's ensure the headerCSS feature works correctly with Radix's styling system.
tailwind.config.js (1)
327-329: LGTM! Animations are properly configured.
The animations are well-configured with appropriate timing (0.2s) and easing function (ease-out) for smooth accordion transitions.
Let's verify that these animation names are used consistently across the codebase:
✅ Verification successful
Animations are properly configured and used correctly
The verification confirms that:
- The accordion animations are properly defined in
tailwind.config.jswith both keyframes and animation configurations - The animations are correctly used in
Accordion.tsxwith the proper data attributes (data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down) - The timing (0.2s) and easing function (ease-out) are appropriate for smooth transitions
- There are no conflicting animation definitions elsewhere in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new accordion animations
# Expected: References in Accordion component styles/classes
# Search for accordion animation class usage
echo "Checking for accordion animation classes usage:"
rg -l "accordion-(up|down)"
# Search for animation configuration in other style files
echo -e "\nChecking for any conflicting accordion animation definitions:"
rg "keyframes.*accordion-(up|down)"
Length of output: 353
Script:
#!/bin/bash
# Let's examine the actual usage in Accordion.tsx and check for keyframes definitions
echo "Checking Accordion.tsx content:"
cat src/core/Accordion.tsx
echo -e "\nChecking keyframes definitions in tailwind config:"
rg "@keyframes accordion-" tailwind.config.js
Length of output: 4566
Script:
#!/bin/bash
# Let's check the keyframes section in tailwind.config.js
echo "Checking complete tailwind.config.js for keyframes:"
cat tailwind.config.js
Length of output: 13395
src/core/Accordion.tsx (6)
1-8: Imports from Radix UI and clsx
The new imports from @radix-ui/react-accordion and clsx are appropriate and correctly implemented.
32-33: Addition of index prop to AccordionRowProps
Introducing the index: number; prop to AccordionRowProps allows each accordion item to have a unique identifier, which is essential for managing state and rendering.
40-42: Extending AccordionProps with HTML div attributes
By extending AccordionProps with React.HTMLAttributes<HTMLDivElement>, you enhance the flexibility of the Accordion component, allowing users to pass standard HTML div attributes.
76-76: Ensure the onClick handler does not interfere with default behavior
Passing an onClick handler to AccordionTrigger should be done cautiously to avoid disrupting the accordion's default toggle functionality.
Please verify that the onClick handler complements the accordion's behavior without causing any side effects. Test to ensure that both the custom onClick action and the default expand/collapse actions operate smoothly.
91-104: Confirm toggle icon visibility and transitions
Ensure that the toggle icons for open and closed states display correctly, and the transition between icons is smooth.
Test the accordion to confirm that:
- The correct icon is displayed when an accordion item is expanded or collapsed.
- The CSS classes controlling visibility (
group-data-[state=closed]/accordion-trigger:hidden, etc.) work as intended.
152-159: Verify the behavior of defaultValue in RadixAccordion
When options?.autoClose is false, the accordion is set to type="multiple" with defaultValue={openIndexes}. Ensure that this correctly initializes the open accordion items.
Test the component to confirm:
- Accordion items specified in
options?.defaultOpenIndexesare open by default. - The
fullyOpenoption opens all accordion items when set totrue. - The accordion behaves as expected when toggling items open and closed.
jamiehenson
force-pushed
the
radix-accordion
branch
from
e99f5f3 to
bf71c2e
Compare
jamiehenson
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/core/Accordion.tsx (2)
69-118: Consider extracting complex className logic.The className logic in AccordionTrigger is quite complex. Consider extracting it to a separate utility function for better maintainability.
+ const getAccordionTriggerClasses = (theme: AccordionTheme, options?: AccordionOptions) => + clsx({ + "flex w-full group/accordion-trigger py-16 ui-text-p1 font-bold text-left items-center gap-12 transition-colors": true, + "px-16 mb-16 rounded-lg": isNonTransparentTheme(theme), + // ... rest of the classes + }); <AccordionTrigger onClick={onClick} - className={clsx({...})} + className={getAccordionTriggerClasses(theme, options)} >
90-111: Consider simplifying toggle icon rendering.The toggle icon implementation could be simplified by using a single Icon component with dynamic props.
- <Icon - additionalCSS="group-data-[state=closed]/accordion-trigger:hidden" - name={toggleIcons.open.name} - color={toggleIconColor} - size={options?.iconSize ?? "16px"} - /> - <Icon - additionalCSS="group-data-[state=open]/accordion-trigger:hidden" - name={toggleIcons.closed.name} - color={toggleIconColor} - size={options?.iconSize ?? "16px"} - /> + <Icon + additionalCSS="transition-transform group-data-[state=open]/accordion-trigger:rotate-180" + name={toggleIcons.closed.name} + color={toggleIconColor} + size={options?.iconSize ?? "16px"} + />src/core/Accordion/Accordion.stories.tsx (1)
231-250: Enhance the custom styling story implementation and documentation.While the story effectively demonstrates the customization capabilities, consider the following improvements:
- Avoid using Tailwind's
!important modifier in the example as it may encourage override-heavy styling patterns.- Consider using responsive height classes instead of fixed
h-40.- Rephrase the documentation to be more professional, focusing on the technical capabilities rather than aesthetic judgments.
- Consider using relative units (rem/em) for icon sizes instead of pixels.
Here's a suggested improvement:
export const WithCustomElementCSS = { render: () => AccordionPresentation({ data: dataWithIcons, options: { - headerCSS: "bg-pink-400 hover:!bg-pink-600 h-40", - iconSize: "40px", - rowIconSize: "12px", + headerCSS: "bg-pink-400 hover:bg-pink-600 min-h-[2.5rem] md:min-h-[3rem]", + iconSize: "2.5rem", + rowIconSize: "0.75rem", }, }), parameters: { docs: { description: { story: - "By modifying `headerCSS`, `iconSize`, and `rowIconSize` in `options`, you can customize the styling of the header, row expansion icons (i.e. the plus and minus), and the icons on the left of the row. What's below is hideous, but you get the gist.", + "Demonstrates header and icon customization capabilities through `headerCSS`, `iconSize`, and `rowIconSize` options. The example shows how to modify header background colors, hover states, heights, and icon dimensions while maintaining responsive design principles.", }, }, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
src/core/Icon/__snapshots__/Icon.stories.tsx.snapis excluded by!**/*.snapsrc/core/icons/icon-gui-chevron-down.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-chevron-up.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-hand.svgis excluded by!**/*.svgsrc/core/icons/icon-product-asset-tracking-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-chat-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-livesync-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-platform-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-pub-sub-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-spaces-mono.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(2 hunks)src/core/Accordion.tsx(2 hunks)src/core/Accordion/Accordion.stories.tsx(2 hunks)src/core/Accordion/types.ts(3 hunks)src/core/Accordion/utils.ts(1 hunks)src/core/Icon/Icon.stories.tsx(1 hunks)src/core/styles/colors/types.ts(1 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- src/core/Accordion/types.ts
- src/core/Accordion/utils.ts
- src/core/Icon/Icon.stories.tsx
- src/core/styles/colors/types.ts
- tailwind.config.js
🔇 Additional comments (4)
src/core/Accordion.tsx (3)
Line range hint 1-42: LGTM! Clean migration to Radix UI components.
The import organization and type definitions are well-structured. The addition of Radix UI components and the new headerCSS property in AccordionProps provides good flexibility for styling.
159-166: LGTM! Well-implemented accordion modes.
The implementation properly handles both single and multiple selection modes using Radix UI's type prop, with correct initialization of default values.
Line range hint 1-166: Great accessibility improvements with Radix UI!
The migration to Radix UI's Accordion components brings several accessibility improvements:
- Built-in keyboard navigation
- Proper ARIA attributes
- Focus management
- Screen reader announcements
src/core/Accordion/Accordion.stories.tsx (1)
60-64: LGTM: Consistent theme-based text styling.
The conditional text color styling for dark themes is well-implemented and maintains consistency with the parent container's theme handling.
jamiehenson
force-pushed
the
radix-accordion
branch
from
863f5c3 to
b90585a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/core/Accordion/types.ts (1)
53-89: Consider using a more specific type for border propertyInstead of using
stringfor the border property, consider using a more specific type likeColorClassor creating a dedicated border color type for better type safety.- border?: string; + border?: ColorClass;src/core/styles/text.css (2)
169-183: Consider aligning naming conventions and reducing style duplication.The new menu label classes (
ui-menu-label-*) follow a different naming pattern compared to existing menu classes (ui-text-menu*). Additionally, they share many properties with existing menu classes but use different color tokens.Consider either:
- Aligning the naming convention with existing patterns:
-.ui-menu-label-1 +.ui-text-menu-label-1
- Or extending existing menu classes to reduce duplication:
.ui-menu-label-1 { @apply ui-text-menu1 text-neutral-1000; }
169-183: Consider adding dark mode support.The text color is hardcoded to
text-neutral-1000without dark mode consideration. This might cause readability issues in dark themes.Consider adding dark mode variants:
.ui-menu-label-1 { - @apply font-sans text-neutral-1000 text-p1 font-medium leading-snug; + @apply font-sans text-neutral-1000 dark:text-neutral-100 text-p1 font-medium leading-snug; }src/core/Accordion.tsx (4)
25-64: Consider enhancing type safety for the onClick callback.The type definition is well-documented and structured. Consider making the onClick callback type more specific by including the event parameter:
- onClick: () => void; + onClick: (event: React.MouseEvent<HTMLButtonElement>) => void;
114-133: Consider memoizing complex class compositions.The clsx class composition is quite complex and recalculated on every render. Consider memoizing it with useMemo:
+ const triggerClasses = useMemo(() => + clsx({ + "flex w-full group/accordion-trigger py-16 ui-text-p1 font-bold text-left items-center gap-12 transition-colors": true, + "px-16 mb-16 rounded-lg": isNonTransparentTheme(theme), + // ... rest of the classes + }), + [theme, isNonTransparentTheme(theme), isStaticTheme(theme), sticky, bg, hoverBg, text, selectableBg, selectableText, options?.headerCSS] + );
120-159: Enhance keyboard navigation accessibility.While Radix provides good accessibility defaults, consider adding explicit ARIA labels for better screen reader support:
<AccordionTrigger onClick={onClick} + aria-label={`Toggle ${name} section`} className={triggerClasses} >
186-188: Add type safety to the onClick handler.The onClick handler should include type checking for the optional callback:
- onClick={() => { - item.onClick?.(index); - }} + onClick={() => { + if (typeof item.onClick === 'function') { + item.onClick(index); + } + }}tailwind.config.js (1)
319-330: Consider accessibility and performance optimizations for animationsWhile the accordion animations are well-implemented and follow Radix UI's patterns, consider these improvements:
- Add reduced motion support for accessibility
- Use transform properties alongside height for better performance
Consider updating the animations like this:
keyframes: { "accordion-down": { - from: { height: "0" }, - to: { height: "var(--radix-accordion-content-height)" }, + from: { height: "0", transform: "translateY(-4px)" }, + to: { height: "var(--radix-accordion-content-height)", transform: "translateY(0)" }, }, "accordion-up": { - from: { height: "var(--radix-accordion-content-height)" }, - to: { height: "0" }, + from: { height: "var(--radix-accordion-content-height)", transform: "translateY(0)" }, + to: { height: "0", transform: "translateY(-4px)" }, }, }, animation: { "accordion-down": "accordion-down 0.2s ease-out", "accordion-up": "accordion-up 0.2s ease-out", + // Add reduced motion variants + "accordion-down-reduced": "accordion-down 0s", + "accordion-up-reduced": "accordion-up 0s", },Also, consider adding a media query in your CSS:
@media (prefers-reduced-motion: reduce) { .animate-accordion-down { animation: accordion-down-reduced; } .animate-accordion-up { animation: accordion-up-reduced; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
src/core/Icon/__snapshots__/Icon.stories.tsx.snapis excluded by!**/*.snapsrc/core/icons/icon-gui-chevron-down.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-chevron-up.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-hand.svgis excluded by!**/*.svgsrc/core/icons/icon-product-asset-tracking-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-chat-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-livesync-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-platform-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-pub-sub-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-spaces-mono.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.json(2 hunks)src/core/Accordion.tsx(2 hunks)src/core/Accordion/Accordion.stories.tsx(2 hunks)src/core/Accordion/types.ts(2 hunks)src/core/Accordion/utils.ts(1 hunks)src/core/Icon/Icon.stories.tsx(1 hunks)src/core/styles/colors/types.ts(1 hunks)src/core/styles/text.css(1 hunks)tailwind.config.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/core/Accordion/Accordion.stories.tsx
- src/core/Accordion/utils.ts
- src/core/Icon/Icon.stories.tsx
- src/core/styles/colors/types.ts
🔇 Additional comments (6)
src/core/Accordion/types.ts (3)
2-3: LGTM! Imports are well-organized for Radix integration
The addition of DataStateOpenColorClass type aligns with Radix UI's data attribute pattern for styling state changes.
5-30: Verify index parameter usage with Radix Accordion
The onClick handler includes an index parameter, but we should verify this aligns with Radix's Accordion implementation and event handling patterns.
✅ Verification successful
Let's search for the actual implementation files and onClick usage patterns to verify the index parameter usage.
Index parameter in onClick handler is correctly implemented
The implementation aligns perfectly with the type definition:
- The
onClickhandler inAccordionDatatype correctly defines(index: number) => void - The Accordion component properly passes the index to the handler:
item.onClick?.(index)in the implementation - The index represents the array position of the accordion item, which is useful for identifying which item was clicked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for onClick usage in Accordion implementation
ast-grep --pattern 'onClick($index) {
$$$
}'
# Search for Radix Accordion usage patterns
rg -A 5 "Accordion.Root|Accordion.Item"
Length of output: 90
Script:
#!/bin/bash
# Search for Accordion component implementation
fd "Accordion" --type f --exec cat {}
# Search for onClick usage in any Accordion-related files
rg -A 5 "onClick.*index" --type ts
Length of output: 12813
93-147: Consider type safety improvements for styling options
- The
headerCSSproperty could benefit from a more specific type to ensure valid CSS classes are used. - Verify that
rowIconSizeandiconSizeare used consistently across the component.
src/core/Accordion.tsx (2)
Line range hint 1-23: LGTM! Well-organized imports with clear separation of concerns.
The transition to Radix UI components and the organization of imports demonstrate good architectural decisions.
67-87: LGTM! Well-structured props interface.
The AccordionProps type is well-documented and provides good extensibility through HTMLDivElement attributes.
tailwind.config.js (1)
29-29: LGTM: Font size addition follows established patterns
The new p4 font size declaration maintains consistency with existing paragraph styles, using appropriate CSS variables for font size and line height.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/core/styles/properties.css (1)
188-188: LGTM! Consider documenting the use case.The new font size variable follows the established naming convention and maintains the consistent size decrement pattern of the paragraph series. Since this introduces a new typography option, consider documenting its intended use case in a comment to help maintain design consistency.
+ /* Used for compact paragraph text in constrained spaces */ --fs-p4: 0.813rem;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/core/icons/icon-product-pubsub-mono.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
package.json(2 hunks)src/core/styles/properties.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/core/Accordion/types.ts (3)
5-29: Documentation looks great, consider adding @exampleThe type definition and documentation are well-structured. Consider enhancing the documentation with an @example section to demonstrate typical usage.
/** * Represents the data structure for an Accordion component. + * @example + * const accordionData: AccordionData = { + * name: "Section 1", + * icon: "chevron-right", + * content: <div>Content here</div>, + * onClick: (index) => console.log(`Clicked item ${index}`) + * }; */
53-89: Consider using a string literal type for hoverBgThe type definition is well-documented and structured. However,
hoverBgis typed as string while other color properties use ColorClass.Consider using ColorClass for consistency:
/** * Background color when the accordion item is hovered. */ - hoverBg: string; + hoverBg: ColorClass;
93-161: Consider grouping related optionsThe AccordionOptions type is well-defined and thoroughly documented. To improve maintainability, consider grouping related options into nested objects.
Example organization:
export type AccordionOptions = { /** Behavior options */ behavior?: { autoClose?: boolean; selectable?: boolean; sticky?: boolean; fullyOpen?: boolean; }; /** Style options */ style?: { headerCSS?: string; selectedHeaderCSS?: string; contentCSS?: string; hideBorders?: boolean; }; /** Icon options */ icons?: { rowIconSize?: IconSize; iconSize?: IconSize; }; /** Initial state */ defaultOpenIndexes?: number[]; };src/core/Accordion.tsx (3)
94-107: Consider extracting complex className logicThe className logic is quite complex with multiple conditions. Consider extracting it into a separate utility function for better maintainability and testability.
+ const getTriggerClassNames = ( + theme: AccordionTheme, + isOpen: boolean, + options?: AccordionOptions, + selectable?: boolean + ) => { + return clsx({ + "flex w-full group/accordion-trigger py-16 ui-text-p1 font-bold text-left items-center gap-12 transition-colors": + true, + "px-16 mb-16 rounded-lg": isNonTransparentTheme(theme), + "rounded-none": !isNonTransparentTheme(theme), + "pointer-events-none focus:outline-none": isStaticTheme(theme), + "focus:outline-gui-blue-focus": !isStaticTheme(theme), + "sticky top-0": options?.sticky, + [`${bg} ${hoverBg} ${text}`]: !(selectable && isOpen), + [`${selectableBg} ${selectableText}`]: selectable && isOpen, + [options?.headerCSS ?? ""]: options?.headerCSS, + [options?.selectedHeaderCSS ?? ""]: + options?.selectedHeaderCSS && isOpen, + }); + }; <AccordionTrigger onClick={onClick} - className={clsx({ - // ... current complex logic - })} + className={getTriggerClassNames(theme, isOpen, options, selectable)} >
160-162: Consider preventing event propagation in onClick handlerThe onClick handler might cause unexpected behavior due to event bubbling. Consider preventing event propagation.
onClick={() => { + event.stopPropagation(); item.onClick?.(index); }}
179-197: Consider adding error boundariesThe Accordion component handles complex state and user interactions. Consider wrapping it with an error boundary to gracefully handle potential runtime errors.
class AccordionErrorBoundary extends React.Component { // ... error boundary implementation } // Usage: <AccordionErrorBoundary> <div {...props}> {options?.autoClose ? ( <RadixAccordion /*...*/> {innerAccordion} </RadixAccordion> ) : ( <RadixAccordion /*...*/> {innerAccordion} </RadixAccordion> )} </div> </AccordionErrorBoundary>src/core/Accordion/Accordion.stories.tsx (1)
231-252: Consider enhancing the custom styling exampleWhile the story effectively demonstrates the customization capabilities, consider the following improvements:
- Use design system color tokens instead of arbitrary colors (e.g.,
pink-400,green-400)- Add a note about maintaining WCAG color contrast requirements
- Consider using semantic height classes or design system spacing tokens instead of
h-40- Provide a more visually appealing example that better represents real-world usage
Here's a suggested improvement:
export const WithCustomElementCSS = { render: () => AccordionPresentation({ data: dataWithIcons, options: { - selectedHeaderCSS: "bg-green-400 hover:bg-blue-600", - contentCSS: "bg-yellow-200", - headerCSS: "bg-pink-400 hover:bg-pink-600 h-40", + selectedHeaderCSS: "bg-primary-100 hover:bg-primary-200", + contentCSS: "bg-neutral-100", + headerCSS: "bg-neutral-200 hover:bg-neutral-300 py-4", iconSize: "40px", rowIconSize: "12px", }, }), parameters: { docs: { description: { story: - "By modifying `headerCSS`, `selectedHeaderCSS`, `contentCSS`, `iconSize`, and `rowIconSize` in `options`, you can customize the styling of the header, row expansion icons (i.e. the plus and minus), and the icons on the left of the row. What's below is hideous, but you get the gist.", + "By modifying `headerCSS`, `selectedHeaderCSS`, `contentCSS`, `iconSize`, and `rowIconSize` in `options`, you can customize the styling of the header, row expansion icons (i.e. the plus and minus), and the icons on the left of the row. Ensure to maintain accessibility standards and use design system tokens when customizing colors and spacing.", }, }, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
package.json(2 hunks)src/core/Accordion.tsx(1 hunks)src/core/Accordion/Accordion.stories.tsx(2 hunks)src/core/Accordion/types.ts(2 hunks)src/core/Accordion/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/core/Accordion/utils.ts
🔇 Additional comments (4)
src/core/Accordion/types.ts (1)
2-2: LGTM: Import changes are appropriate
The addition of IconSize import is well-aligned with its usage in the new AccordionOptions type properties.
src/core/Accordion.tsx (2)
1-56: LGTM! Well-structured imports and type definitions
The type definitions are comprehensive and well-documented with JSDoc comments. The transition to Radix UI components is cleanly implemented.
86-136: Verify accessibility and performance
While Radix UI provides good accessibility features by default, please ensure:
- ARIA attributes are properly passed through to the final DOM
- Animations perform well on lower-end devices
- Keyboard navigation works as expected
Also applies to: 179-197
src/core/Accordion/Accordion.stories.tsx (1)
60-64: LGTM: Theme-aware text color implementation
The conditional text color application for dark themes improves visibility and maintains consistency with the theme system.
jamiehenson
force-pushed
the
radix-accordion
branch
from
efc1725 to
6e3ee3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/core/Accordion/types.ts (2)
5-29: Documentation looks great, minor enhancement suggestedThe type definition and documentation are well-structured. Consider adding an
@examplesection to the JSDoc comments to demonstrate typical usage.Here's a suggested enhancement for the documentation:
/** * Represents the data structure for an Accordion component. + * @example + * const accordionData: AccordionData = { + * name: "Section 1", + * icon: "chevron-right", + * content: <div>Content here</div>, + * onClick: (index) => console.log(`Clicked item ${index}`) + * }; */
93-161: Well-structured options type with comprehensive documentationThe AccordionOptions type is well-designed and thoroughly documented. Consider extracting the CSS-related options into a separate interface for better organization and reusability.
Consider this refactor:
interface AccordionStyleOptions { headerCSS?: string; selectedHeaderCSS?: string; contentCSS?: string; } export type AccordionOptions = AccordionStyleOptions & { autoClose?: boolean; selectable?: boolean; // ... other non-CSS options };src/core/Accordion.tsx (2)
94-107: Consider extracting complex className conditions into a separate function.The className logic in AccordionTrigger is quite complex and could benefit from being extracted into a utility function for better maintainability.
Consider refactoring like this:
+ const getAccordionTriggerClasses = ( + theme: AccordionTheme, + isOpen: boolean, + options?: AccordionOptions, + selectable?: boolean + ) => ({ + "flex w-full group/accordion-trigger py-16 ui-text-p1 font-bold text-left items-center gap-12 transition-colors": true, + "px-16 mb-16 rounded-lg": isNonTransparentTheme(theme), + "rounded-none": !isNonTransparentTheme(theme), + "pointer-events-none focus:outline-none": isStaticTheme(theme), + "focus:outline-gui-blue-focus": !isStaticTheme(theme), + "sticky top-0": options?.sticky, + [`${bg} ${hoverBg} ${text}`]: !(selectable && isOpen), + [`${selectableBg} ${selectableText}`]: selectable && isOpen, + [options?.headerCSS ?? ""]: options?.headerCSS, + [options?.selectedHeaderCSS ?? ""]: options?.selectedHeaderCSS && isOpen, + }); - className={clsx({ - "flex w-full group/accordion-trigger py-16 ui-text-p1 font-bold text-left items-center gap-12 transition-colors": true, - // ... rest of conditions - })} + className={clsx(getAccordionTriggerClasses(theme, isOpen, options, selectable))}
1-197: Great architectural improvement using Radix UI!The transition to Radix UI's Accordion component brings several benefits:
- Improved accessibility through Radix's built-in ARIA attributes
- Reduced maintenance burden by leveraging battle-tested components
- Better animation handling with CSS transitions
- Cleaner state management with built-in selection modes
This aligns well with the PR objectives of reducing maintenance burden while enhancing functionality.
src/core/Accordion/Accordion.stories.tsx (1)
231-252: Enhance the custom styling story with design system alignmentWhile the story effectively demonstrates the customization capabilities, consider these improvements:
- Use the UI library's color system instead of arbitrary color classes (e.g., replace
bg-green-400with design system colors)- Align height values with the design system's spacing scale
- Add accessibility considerations to the documentation
- Rephrase the documentation to maintain professionalism (avoid terms like "hideous")
Here's a suggested improvement:
export const WithCustomElementCSS = { render: () => AccordionPresentation({ data: dataWithIcons, options: { - selectedHeaderCSS: "bg-green-400 hover:bg-blue-600", - contentCSS: "bg-yellow-200", - headerCSS: "bg-pink-400 hover:bg-pink-600 h-40", + selectedHeaderCSS: "bg-neutral-200 hover:bg-neutral-300", + contentCSS: "bg-neutral-100", + headerCSS: "bg-primary-100 hover:bg-primary-200 h-48", iconSize: "40px", rowIconSize: "12px", }, }), parameters: { docs: { description: { story: - "By modifying `headerCSS`, `selectedHeaderCSS`, `contentCSS`, `iconSize`, and `rowIconSize` in `options`, you can customize the styling of the header, row expansion icons (i.e. the plus and minus), and the icons on the left of the row. What's below is hideous, but you get the gist.", + "The Accordion component supports extensive styling customization through the following options:\n" + + "- `headerCSS`: Customize the header's appearance\n" + + "- `selectedHeaderCSS`: Style the selected header state\n" + + "- `contentCSS`: Modify the content section styling\n" + + "- `iconSize`: Set the size of the main icons\n" + + "- `rowIconSize`: Adjust the expansion indicator size\n\n" + + "Note: When customizing styles, ensure maintaining sufficient color contrast for accessibility.", }, }, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
src/core/Icon/__snapshots__/Icon.stories.tsx.snapis excluded by!**/*.snapsrc/core/icons/icon-gui-chevron-down.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-chevron-up.svgis excluded by!**/*.svgsrc/core/icons/icon-gui-hand.svgis excluded by!**/*.svgsrc/core/icons/icon-product-asset-tracking-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-chat-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-livesync-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-platform-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-pubsub-mono.svgis excluded by!**/*.svgsrc/core/icons/icon-product-spaces-mono.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.json(2 hunks)src/core/Accordion.tsx(1 hunks)src/core/Accordion/Accordion.stories.tsx(2 hunks)src/core/Accordion/types.ts(2 hunks)src/core/Accordion/utils.ts(1 hunks)src/core/Icon/Icon.stories.tsx(1 hunks)src/core/styles/properties.css(1 hunks)src/core/styles/text.css(1 hunks)tailwind.config.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- src/core/Accordion/utils.ts
- src/core/Icon/Icon.stories.tsx
- src/core/styles/properties.css
- src/core/styles/text.css
- tailwind.config.js
🔇 Additional comments (5)
src/core/Accordion/types.ts (2)
2-2: LGTM: Import changes are appropriate
The addition of IconSize import is well-aligned with its usage in the new AccordionOptions type.
53-90: Verify type consistency for color properties
The type definition uses a mix of ColorClass and string types for colors. While this provides flexibility, we should verify that this is intentional and consistent with the design system.
✅ Verification successful
Mixed color type usage aligns with the design system's flexibility
The combination of ColorClass and string types is consistent with the design system's flexibility in allowing both predefined classes and custom string values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency in color type usage across the codebase
echo "Checking color type usage patterns..."
# Search for ColorClass usage
echo "ColorClass usage:"
rg "ColorClass" -A 2 -B 2
# Search for direct string color properties
echo "Direct string color usage:"
rg "color:.*string" -A 2 -B 2
Length of output: 9218
src/core/Accordion.tsx (2)
1-56: LGTM! Well-structured types and imports.
The type definitions are comprehensive and well-documented. The transition to Radix UI components is cleanly implemented with proper type safety.
169-176: LGTM! Proper useMemo implementation with correct dependencies.
The state management for open indexes is well implemented with proper memoization and dependencies.
src/core/Accordion/Accordion.stories.tsx (1)
60-64: LGTM: Improved theme-based text color handling
The conditional application of text color based on theme improves visual consistency.
jamiehenson
changed the title
| "addsearch-js-client": "^0.8.11", | ||
| "array-flat-polyfill": "^1.0.1", | ||
| "clsx": "^2.1.1", |
There was a problem hiding this comment.
nice, I forgot about this package. and it's even in Voltaire already. will bear in mind to use this next time :D
There was a problem hiding this comment.
Yeah I forgot about it as well! Much cleaner than doing loads string interpolation as we have been doing elsewhere (and as you say we already have this package elsewhere). From what I can see clsx and classnames are basically interchangeable, so we need one or other other (though I understand that clsx is just the same but lighter)
jamiehenson
force-pushed
the
radix-accordion
branch
from
6e3ee3c to
405f9de
Compare
Jira Ticket Link / Motivation
Related to https://ably.atlassian.net/browse/WEB-3926, provides things the new docs nav needs.
Summary of changes
Radix
We don't need a dedicated
Menucomponent or anything like that, we can repurpose our existingAccordioncomponent. In this PR, I've switched the underlying foundation ofAccordionto use Radix's Accordion component - purely to eliminate the maintenance burden of keeping up with the fundamentals when there is already a fully formed, light primitive already out there.Going forward, I will investigate Radix alternatives for other components - the Tooltip is also a good candidate.
Accordion props
Additionally, I've added a few icons that the new docs nav needs, and the following props to the
optionsblock, each of which adds further styling to the section the names depict:headerCSSselectedHeaderCSScontentCSSOther
I've also added TypeDoc references to Accordion types so that helpful descriptions can be surfaced downstream in tooltips in IDEs - I saw that Radix does this and thought it was a neat convention to adopt with our stuff as well.
Furthermore, I've added classes to represent the menu typography styling in the design system (i.e.
ui-menu-label-4).How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style
Chores