Featue controlled/uncontrolled modes for accordian #1031
Conversation
WalkthroughThe changes refine the accordion components by establishing a more explicit association between data and rendered items. In the example file, each accordion item now receives a specific Changes
Sequence Diagram(s)sequenceDiagram
participant AE as AccordionExample
participant AI as Accordion.Item
AE->>AE: Iterate over items with fixed IDs
AE->>AI: Render each item using its `id` ("react", "angular", "vue")
sequenceDiagram
participant AR as AccordionRoot
participant AC as AccordionContext.Provider
participant Child as Child Components
AR->>AC: Pass props (value, defaultValue, onValueChange)
AC->>Child: Provide state and update callbacks
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
npm warn config production Use ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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/Accordion/fragments/AccordionRoot.tsx (1)
30-30: 🛠️ Refactor suggestionType inconsistency in state management
The
activeItemsstate uses(number | string)[]but the new props usestring | string[]. This inconsistency could lead to unexpected behavior.Consider updating the state type to match the props:
- const [activeItems, setActiveItems] = useState<(number | string)[]>([]); + const [activeItems, setActiveItems] = useState<string[]>([]);This change would require updating any code that relies on numeric values in
activeItems.
🧹 Nitpick comments (2)
docs/app/docs/components/accordion/docs/example_1.tsx (1)
27-36: Consider demonstrating controlled modeThe example doesn't demonstrate the new controlled mode capabilities added to the
AccordionRootcomponent. Consider updating this example to show how to use the newvalue,defaultValue, andonValueChangeprops.- <Accordion.Root> + <Accordion.Root + defaultValue="react" + // Or for controlled mode: + // value={selectedItem} + // onValueChange={setSelectedItem} + >This would help document the new functionality and provide a usage example for other developers.
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
12-24: Add documentation for the new propsConsider adding JSDoc comments to explain how the new controlled mode props work.
export type AccordionRootProps = { children: React.ReactNode; customRootClass?: string; transitionDuration?: number; transitionTimingFunction?: string; orientation?: 'horizontal' | 'vertical'; asChild?: boolean; loop?: boolean; openMultiple?: boolean; + /** + * The controlled value of the accordion. Can be a single string or an array of strings. + * Use with onValueChange for controlled mode. + */ value?: string | string[]; + /** + * The default value for the accordion in uncontrolled mode. + * Can be a single string or an array of strings. + */ defaultValue?: string | string[]; + /** + * Callback fired when the value changes. + * Returns a single string if openMultiple is false, or an array of strings if openMultiple is true. + */ onValueChange?: (value: string | string[]) => void }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/app/docs/components/accordion/docs/example_1.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
docs/app/docs/components/accordion/docs/example_1.tsx
[error] 29-29: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (4)
docs/app/docs/components/accordion/docs/example_1.tsx (2)
4-20: Good addition of explicit IDs to itemsAdding unique IDs to each item is a good practice as it moves away from index-based identification. This approach provides more stable references for controlled components.
31-33: Good removal of index-based propsRemoving index-based props from the trigger and content components is appropriate when using unique IDs from the data model instead.
src/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
21-23: Good addition of controlled mode propsThe addition of
value,defaultValue, andonValueChangeprops enables both controlled and uncontrolled modes for the Accordion component, providing greater flexibility to users.
26-26: Function signature updated correctlyThe function signature has been properly updated to include the new props.
| {items.map((item) => ( | ||
| <Accordion.Item value={`${item.id}`}> |
There was a problem hiding this comment.
Add a key prop to the iterable element
When rendering lists in React, each element should have a unique key prop to help React identify which items have changed.
- <Accordion.Item value={`${item.id}`}>
+ <Accordion.Item key={item.id} value={item.id}>Also note that the template literal is unnecessary since item.id is already a string.
📝 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.
| {items.map((item) => ( | |
| <Accordion.Item value={`${item.id}`}> | |
| {items.map((item) => ( | |
| <Accordion.Item key={item.id} value={item.id}> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
| openMultiple, | ||
| value, | ||
| defaultValue, | ||
| onValueChange |
There was a problem hiding this comment.
Props passed to context, but implementation missing
While the props are correctly passed to the context, there's no implementation to handle controlled behavior. The component needs logic to:
- Initialize state from
defaultValue - Update state when
valuechanges - Call
onValueChangewhen the state changes
Consider implementing the controlled behavior with a useEffect hook:
const AccordionRoot = ({ children, orientation = 'vertical', asChild, transitionDuration = 0, transitionTimingFunction = 'linear', customRootClass, loop = true, openMultiple = false, value, defaultValue, onValueChange }: AccordionRootProps) => {
const accordionRef = useRef<HTMLDivElement | null>(null);
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
- const [activeItems, setActiveItems] = useState<(number | string)[]>([]);
+ const [activeItems, setActiveItems] = useState<string[]>(() => {
+ // Initialize from defaultValue if provided
+ if (defaultValue !== undefined) {
+ return Array.isArray(defaultValue) ? defaultValue : [defaultValue];
+ }
+ return [];
+ });
+
+ // Handle controlled mode
+ useEffect(() => {
+ if (value !== undefined) {
+ const newActiveItems = Array.isArray(value) ? value : [value];
+ setActiveItems(newActiveItems);
+ }
+ }, [value]);
+
+ // Create wrapper for setActiveItems that calls onValueChange
+ const handleSetActiveItems = (items: string[]) => {
+ setActiveItems(items);
+ if (onValueChange) {
+ onValueChange(openMultiple ? items : items[0] || '');
+ }
+ };Also, you'll need to update how setActiveItems is used throughout the component to use handleSetActiveItems instead.
📝 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.
| openMultiple, | |
| value, | |
| defaultValue, | |
| onValueChange | |
| const AccordionRoot = ({ children, orientation = 'vertical', asChild, transitionDuration = 0, transitionTimingFunction = 'linear', customRootClass, loop = true, openMultiple = false, value, defaultValue, onValueChange }: AccordionRootProps) => { | |
| const accordionRef = useRef<HTMLDivElement | null>(null); | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| const [activeItems, setActiveItems] = useState<string[]>(() => { | |
| // Initialize from defaultValue if provided | |
| if (defaultValue !== undefined) { | |
| return Array.isArray(defaultValue) ? defaultValue : [defaultValue]; | |
| } | |
| return []; | |
| }); | |
| // Handle controlled mode | |
| useEffect(() => { | |
| if (value !== undefined) { | |
| const newActiveItems = Array.isArray(value) ? value : [value]; | |
| setActiveItems(newActiveItems); | |
| } | |
| }, [value]); | |
| // Create wrapper for setActiveItems that calls onValueChange | |
| const handleSetActiveItems = (items: string[]) => { | |
| setActiveItems(items); | |
| if (onValueChange) { | |
| onValueChange(openMultiple ? items : items[0] || ''); | |
| } | |
| }; | |
| // ... rest of the component logic. | |
| // Update usage of setActiveItems elsewhere in the component to handleSetActiveItems | |
| }; |
There was a problem hiding this comment.
agreed, implementation missing
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Accordion-related files to understand the component structure
echo "Accordion-related files:"
fd "Accordion" src/components/ui
# Check the AccordionContext file to see how state is currently managed
echo "\nAccordionContext content:"
cat src/components/ui/Accordion/contexts/AccordionContext.tsx
# Look at AccordionItem implementation to see how it interacts with context
echo "\nAccordionItem content:"
fd "AccordionItem.tsx" src/components/ui --exec cat {}
Length of output: 3911
@kotAPI I'd be happy to help implement the missing functionality. Based on my analysis, we need to make the following changes:
- Update the AccordionContext type definition to include the new props
- Implement the controlled/uncontrolled behavior in AccordionRoot
Here's a complete implementation:
// First, update the AccordionContext type in src/components/ui/Accordion/contexts/AccordionContext.tsx
interface AccordionContextType {
rootClass?: string | null;
activeItems: (number | string)[];
setActiveItems: (items: (number | string)[]) => void;
accordionRef?: React.RefObject<HTMLDivElement | null>;
transitionDuration?: number;
transitionTimingFunction?: string;
openMultiple?: boolean;
// Add the new props
value?: string | string[];
defaultValue?: string | string[];
onValueChange?: (value: string | string[]) => void;
}
// Then, update the AccordionRoot component implementation
import React, { useState, useRef, useEffect } from 'react';
// ... other imports
const AccordionRoot = ({
children,
orientation = 'vertical',
asChild,
transitionDuration = 0,
transitionTimingFunction = 'linear',
customRootClass,
loop = true,
openMultiple = false,
value,
defaultValue,
onValueChange
}: AccordionRootProps) => {
const accordionRef = useRef<HTMLDivElement | null>(null);
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
const isControlled = value !== undefined;
// Initialize state from defaultValue if provided
const [activeItems, setActiveItemsState] = useState<(number | string)[]>(() => {
if (defaultValue !== undefined) {
return Array.isArray(defaultValue) ? defaultValue : [defaultValue];
}
return [];
});
// Handle controlled mode
useEffect(() => {
if (isControlled) {
const newActiveItems = Array.isArray(value) ? value : value ? [value] : [];
setActiveItemsState(newActiveItems);
}
}, [value, isControlled]);
// Create wrapper for setActiveItems that calls onValueChange
const setActiveItems = (items: (number | string)[]) => {
if (!isControlled) {
setActiveItemsState(items);
}
if (onValueChange) {
onValueChange(openMultiple ? items : items[0] || '');
}
};
return (
<AccordionContext.Provider
value={{
rootClass,
activeItems,
setActiveItems,
accordionRef,
transitionDuration,
transitionTimingFunction,
openMultiple,
value,
defaultValue,
onValueChange
}}>
<RovingFocusGroup.Root orientation={orientation} loop={loop}>
<RovingFocusGroup.Group >
<Primitive.div className={clsx(`${rootClass}-root`)} ref={accordionRef} asChild={asChild}>
{children}
</Primitive.div>
</RovingFocusGroup.Group>
</RovingFocusGroup.Root>
</AccordionContext.Provider>
);
};This implementation:
- Handles initialization from
defaultValue - Updates state when
valuechanges (controlled mode) - Properly calls
onValueChangewhen state changes - Prevents state updates when in controlled mode (letting the parent component control the state)
Would you like me to also show how to update other components that might be affected by this change?
🧬 Code Definitions Used
Relevant Code Snippets
Filename: src/components/ui/Accordion/contexts/AccordionContext.tsx
export const AccordionContext = createContext<AccordionContextType>({
rootClass: '',
activeItems: [],
setActiveItems: () => {},
accordionRef: undefined,
transitionDuration: 0,
transitionTimingFunction: 'ease-out',
openMultiple: false
});- Description: This snippet defines the
AccordionContextusing React'screateContext. It initializes the context with default values for properties such asrootClass,activeItems,setActiveItems,accordionRef,transitionDuration,transitionTimingFunction, andopenMultiple. This context is used in theAccordionRootcomponent to manage state and provide necessary values to its children.
Filename: src/components/ui/Progress/Progress.tsx
export const COMPONENT_NAME = 'Progress';- Description: This snippet defines a constant
COMPONENT_NAMEfor theProgresscomponent. While it is not directly related to theAccordionRoot, it follows a similar pattern of defining a component name, which may be relevant for consistency in naming conventions across components.
✏️ Learnings added
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.706Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
I added support for props in
AccordionRootfor the user to control the accordion componentSummary by CodeRabbit
New Features
Documentation