feat(Accordion): new promo version #1437
Conversation
| !isControlled && setShouldBeVisible(false); | ||
| } else { | ||
| onOpen?.(); | ||
| !isControlled && setShouldBeVisible(true); |
There was a problem hiding this comment.
You do not need to duplicate the setter, use isExpanded value as an argument for this function
| if (!isExpanded && (event.key === 'Enter' || event.key === ' ')) { | ||
| handleExpandChange(isExpanded); | ||
| } | ||
|
|
||
| if (isExpanded && event.key === 'Escape') { | ||
| handleExpandChange(isExpanded); | ||
| } |
There was a problem hiding this comment.
what is the difference between those if's?
| interface UseAccordionProps { | ||
| isControlled: boolean; | ||
| isExpanded: boolean; | ||
| setShouldBeVisible: React.Dispatch<React.SetStateAction<boolean>>; |
There was a problem hiding this comment.
You can make it a simple callback with a parameter, so you do not need a react dispatch types here.
packages/react-components/src/components/Accordion/Accordion.tsx
Outdated
Show resolved
Hide resolved
WalkthroughThe changes in this pull request enhance the Accordion component by introducing a new promotional variant and improving documentation. Key updates include the addition of new sections in the documentation, new styles for promotional use, and the creation of a custom hook to manage accordion state. The Accordion component is refactored into two components: Changes
Assessment against linked issues
🪧 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
🧹 Outside diff range and nitpick comments (8)
packages/react-components/src/components/Accordion/hooks.ts (1)
3-14: Improve interface naming consistency and documentation
- Remove the 'I' prefix from
IUseAccordionto follow modern TypeScript conventions- Consider renaming
setShouldBeVisibletosetExpandedfor clarityinterface UseAccordionProps { isControlled: boolean; isExpanded: boolean; - setShouldBeVisible: (isVisible: boolean) => void; + setExpanded: (isExpanded: boolean) => void; onOpen?: () => void; onClose?: () => void; } -interface IUseAccordion { +interface UseAccordionReturn { handleExpandChange: (isExpanded: boolean) => void; handleKeyDown: (event: React.KeyboardEvent<HTMLDivElement>) => void; }packages/react-components/src/components/Accordion/types.ts (2)
44-49: Consider using an enum for kind variantsThe interface structure is good, but consider using an enum for the kind variants to improve type safety and maintainability.
+export enum AccordionKind { + Default = 'default', + Warning = 'warning', + Error = 'error' +} export interface IAccordionProps extends IAccordionGlobalProps { /** * Specify the kind of the accordion */ - kind?: 'default' | 'warning' | 'error'; + kind?: AccordionKind; }
51-52: Document the purpose of IAccordionPromoPropsEven if the interface is currently empty, add JSDoc comments to explain its purpose and future extensibility.
+/** + * Interface for the promotional variant of Accordion + * Extends global props without additional properties + */ export interface IAccordionPromoProps extends IAccordionGlobalProps {}packages/react-components/src/components/Accordion/Accordion.mdx (2)
24-31: Add prop descriptions for better clarityConsider adding brief descriptions for
multilineElementandfooterprops to explain their purpose and usage.<Accordion label={{ closed: (<div>Label for closed accordion</div>), open: (<div>Label for open accordion</div>), }} + // Displays additional content below the label multilineElement={<div>Multiline element</div>} + // Renders at the bottom of the expanded accordion footer={<div>Footer element</div>} >
36-55: Highlight PromoAccordion's unique featuresThe example effectively shows the implementation but could benefit from a brief description of how PromoAccordion differs from the default variant.
### Promo Accordion <a id="Promo" /> + +The Promo variant of Accordion provides enhanced styling options optimized for promotional content. +Key differences from the default variant include: +- Custom styling for promotional content +- Specialized layout for marketing materialspackages/react-components/src/components/Accordion/Accordion.module.scss (1)
108-114: Consider adding transition to footerThe footer section looks good, but could benefit from a smooth transition like other elements.
&__footer { border-top: 1px solid var(--border-basic-secondary); padding: var(--spacing-5); + transition: all var(--transition-duration-moderate-1); &--promo { padding: var(--spacing-6); } }packages/react-components/src/components/Accordion/Accordion.spec.tsx (1)
88-97: LGTM! Consider adding edge cases.The test case effectively verifies the footer visibility toggle behavior.
Consider adding these test scenarios:
it('should not show footer when accordion is closed after being open', async () => { const { queryByText, getByRole } = renderComponent({ ...DEFAULT_PROPS, footer: <div>Footer</div>, }); userEvent.click(getByRole('button')); // open await waitFor(() => expect(queryByText('Footer')).toBeInTheDocument()); userEvent.click(getByRole('button')); // close await waitFor(() => expect(queryByText('Footer')).not.toBeInTheDocument()); }); it('should show footer immediately when openOnInit is true', () => { const { queryByText } = renderComponent({ ...DEFAULT_PROPS, footer: <div>Footer</div>, openOnInit: true, }); expect(queryByText('Footer')).toBeInTheDocument(); });packages/react-components/src/components/Accordion/Accordion.stories.tsx (1)
118-122: Enhance footer example with realistic contentConsider enriching the footer example with more realistic content like action buttons or links that demonstrate typical usage patterns.
- <Accordion label="Default" footer={<div>Example footer element</div>}> + <Accordion + label="Default" + footer={ + <div className="accordion-footer"> + <button onClick={() => console.log('Primary action')}> + Save Changes + </button> + <a href="#">Learn more</a> + </div> + } + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/react-components/src/components/Accordion/Accordion.mdx(1 hunks)packages/react-components/src/components/Accordion/Accordion.module.scss(4 hunks)packages/react-components/src/components/Accordion/Accordion.spec.tsx(1 hunks)packages/react-components/src/components/Accordion/Accordion.stories.css(1 hunks)packages/react-components/src/components/Accordion/Accordion.stories.tsx(2 hunks)packages/react-components/src/components/Accordion/Accordion.tsx(4 hunks)packages/react-components/src/components/Accordion/hooks.ts(1 hunks)packages/react-components/src/components/Accordion/index.ts(1 hunks)packages/react-components/src/components/Accordion/types.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-components/src/components/Accordion/Accordion.stories.css
🔇 Additional comments (12)
packages/react-components/src/components/Accordion/index.ts (2)
1-1: Clean and standard export syntax
The barrel export follows best practices for component organization.
1-1: Verify AccordionPromo implementation
Let's ensure the new component is properly defined and documented.
✅ Verification successful
AccordionPromo implementation is complete and well-documented
The component is properly implemented with:
- Full TypeScript interface definition
- Comprehensive documentation in MDX
- Multiple usage examples in stories
- Proper component implementation with all necessary props
- Correct export setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AccordionPromo implementation and documentation
# Check if AccordionPromo is defined in Accordion.tsx
ast-grep --pattern 'export const AccordionPromo = $_'
# Check for documentation
fd -e mdx -e md . | xargs rg -l "AccordionPromo"
Length of output: 894
Script:
#!/bin/bash
# Let's check the actual implementation of AccordionPromo component
ast-grep --pattern 'export const AccordionPromo: React.FC = $_'
# Check the content of the documentation file
cat packages/react-components/src/components/Accordion/Accordion.mdx
# Check if AccordionPromo is properly exported from the main file
ast-grep --pattern 'export { $$$, AccordionPromo, $$$ } from $_'
Length of output: 1742
Script:
#!/bin/bash
# Let's check the actual implementation in the Accordion.tsx file
cat packages/react-components/src/components/Accordion/Accordion.tsx
# And check if there are any tests for AccordionPromo
fd -e test -e spec . | xargs rg -l "AccordionPromo"
Length of output: 4726
Script:
#!/bin/bash
# Let's check the types for AccordionPromo
cat packages/react-components/src/components/Accordion/types.ts
# And check the stories implementation
cat packages/react-components/src/components/Accordion/Accordion.stories.tsx
Length of output: 7646
packages/react-components/src/components/Accordion/types.ts (1)
Line range hint 9-25: LGTM! Verify interface rename impact
The rename to IAccordionGlobalProps and addition of footer property are well structured.
✅ Verification successful
Interface rename is properly implemented
The rename from IAccordionProps to IAccordionGlobalProps is correctly handled:
IAccordionPropsnow extendsIAccordionGlobalProps- All imports and usages are consistent with this hierarchy
- No orphaned exports or imports found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify all usages of the renamed interface have been updated
rg "IAccordionProps" --type ts
Length of output: 619
Script:
#!/bin/bash
# Check if IAccordionProps is used in any exports or imports
ast-grep --pattern 'export { $$$ IAccordionProps $$$ }'
ast-grep --pattern 'import { $$$ IAccordionProps $$$ }'
# Check the implementation to understand the interface hierarchy
ast-grep --pattern 'interface IAccordionProps extends IAccordionGlobalProps { $$$ }'
Length of output: 740
packages/react-components/src/components/Accordion/Accordion.mdx (1)
9-9: LGTM! Clear navigation structure
The navigation links provide easy access to all major sections of the documentation.
packages/react-components/src/components/Accordion/Accordion.module.scss (4)
42-45: LGTM! Proper use of design system variables
The promotional variant styling maintains consistency with the design system.
68-70: Verify chevron positioning with design specs
The chevron's top position is adjusted by 4px. Please ensure this matches the Figma specs.
82-85: LGTM! Consistent spacing usage
The padding adjustments use proper design system spacing variables.
102-104: LGTM! Consistent padding with label
The content padding matches the label's padding, maintaining visual harmony.
packages/react-components/src/components/Accordion/Accordion.stories.tsx (4)
10-15: Clean component organization!
The Typography imports and AccordionPromo subcomponent registration align perfectly with the new promo variant requirements.
Also applies to: 23-25
127-133: Basic example looks good!
Clean implementation of the basic AccordionPromo story.
135-194: Well-structured examples with comprehensive coverage
The examples effectively demonstrate all component variants and features. Good use of Typography components for consistent styling.
143-149: Consider accessibility implications of heading structure
The as="div" with size="xs" might affect document outline. Consider using semantic heading levels that match the content hierarchy.
packages/react-components/src/components/Accordion/Accordion.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/react-components/src/components/Accordion/Accordion.tsx (1)
62-75: Simplify buildHeader implementationThe props spreading logic can be simplified by moving common props to a base object.
const buildHeader = (isPromo?: boolean) => { const Component = isPromo ? Heading : Text; - const props = { + const baseProps = { 'aria-expanded': isExpanded, as: 'div', className: cx(styles[`${baseClass}__label`], { [styles[`${baseClass}__label--promo`]]: isPromo, }), onClick: () => handleExpandChange(isExpanded), - bold: !isPromo ? true : undefined, - ...(isPromo ? { size: 'xs' as TTextSize } : {}), }; + const props = isPromo + ? { ...baseProps, size: 'xs' as TTextSize } + : { ...baseProps, bold: true }; return <Component {...props}>{getLabel(label, isExpanded)}</Component>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/react-components/src/components/Accordion/Accordion.tsx(4 hunks)packages/react-components/src/components/Accordion/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-components/src/components/Accordion/types.ts
🔇 Additional comments (2)
packages/react-components/src/components/Accordion/Accordion.tsx (2)
136-150: Well-structured component separation
The split into separate Accordion and AccordionPromo components provides better type safety and maintainability than parameterization.
Line range hint 79-83: Move keyboard handler to interactive elements
The onKeyDown handler should be on the same elements that have role="button" for proper keyboard accessibility.
This issue was previously raised and needs to be addressed. Move the onKeyDown handler from the outer div to the header component in the buildHeader function.
packages/react-components/src/components/Accordion/Accordion.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/react-components/src/components/Accordion/Accordion.tsx (3)
136-136: Move constant declaration to the topThe
baseClassconstant should be defined before the components for better code organization.-const baseClass = 'accordion'; // current position +const baseClass = 'accordion'; // move to top, after imports const AccordionComponent: React.FC<IAccordionComponentProps> = ({
62-75: Memoize buildHeader functionThe
buildHeaderfunction is recreated on every render but its logic depends only onisPromo,isExpanded, andlabel.+const buildHeader = React.useCallback( (isPromo?: boolean) => { const Component = isPromo ? Heading : Text; const props = { 'aria-expanded': isExpanded, as: 'div', className: cx(styles[`${baseClass}__label`], { [styles[`${baseClass}__label--promo`]]: isPromo, }), onClick: () => handleExpandChange(isExpanded), bold: !isPromo ? true : undefined, ...(isPromo ? { size: 'xs' as TTextSize } : {}), }; return <Component {...props}>{getLabel(label, isExpanded)}</Component>; }, + [isExpanded, label, handleExpandChange] +);
Line range hint
79-84: Move keyboard handler to interactive elementsThe
onKeyDownhandler should be on the same elements that have the interactive roles (the header components) rather than the outer div.<div tabIndex={0} - aria-expanded={isExpanded} className={mergedClassName} - onKeyDown={handleKeyDown} {...props} >Then update the
buildHeaderfunction:const props = { 'aria-expanded': isExpanded, as: 'div', className: cx(styles[`${baseClass}__label`], { [styles[`${baseClass}__label--promo`]]: isPromo, }), onClick: () => handleExpandChange(isExpanded), + onKeyDown: handleKeyDown, bold: !isPromo ? true : undefined, ...(isPromo ? { size: 'xs' as TTextSize } : {}), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/react-components/src/components/Accordion/Accordion.spec.tsx(3 hunks)packages/react-components/src/components/Accordion/Accordion.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-components/src/components/Accordion/Accordion.spec.tsx
🔇 Additional comments (1)
packages/react-components/src/components/Accordion/Accordion.tsx (1)
138-152: Clean implementation of component variants
The separation into Accordion and AccordionPromo components with shared base functionality is well done. This addresses the previous feedback about component parameterization.
Resolves: #1346
Description
A new variant of the
Accordioncomponent as a separate component.Storybook
https://feature-1346--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Release Notes
New Features
Promo Accordionvariant with enhanced styling and functionality.Default AccordionandPromo Accordionwith example implementations.Bug Fixes
Documentation
Style
Chores
AccordionPromocomponent.