Skip to content

Featue controlled/uncontrolled modes for accordian #1031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Featue controlled/uncontrolled modes for accordian #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecf5f9d
improve accordian example implementing ids for value prop
romeu-maleiane Apr 5, 2025
f6646cf
remove key prop from accordion example docs
romeu-maleiane Apr 6, 2025
c9e3526
add support for props
romeu-maleiane Apr 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions docs/app/docs/components/accordion/docs/example_1.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ import Accordion from "@radui/ui/Accordion";

const items = [
{
id: "react",
title: "React",
content: "React is a JavaScript library for building user interfaces."
},
{
id: "angular",
title: "Angular",
content: "Angular is a platform and framework for building single-page client applications using HTML and TypeScript."
},
{
id: "vue",
title: "Vue",
content: "Vue.js is a progressive framework for building user interfaces."
}
Expand All @@ -22,12 +25,12 @@ const AccordionExample = () => {
return (
<div className="w-64 md:w-96">
<Accordion.Root>
{items.map((item, index) => (
<Accordion.Item key={index} value={`item-${index}`}>
{items.map((item) => (
<Accordion.Item value={`${item.id}`}>
Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
{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)

<Accordion.Header>
<Accordion.Trigger index={index}>{item.title}</Accordion.Trigger>
<Accordion.Trigger>{item.title}</Accordion.Trigger>
</Accordion.Header>
<Accordion.Content index={index}>{item.content}</Accordion.Content>
<Accordion.Content>{item.content}</Accordion.Content>
</Accordion.Item>
))}
</Accordion.Root>
Expand Down
10 changes: 8 additions & 2 deletions src/components/ui/Accordion/fragments/AccordionRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ export type AccordionRootProps = {
asChild?: boolean;
loop?: boolean;
openMultiple?: boolean;
value?: string | string[];
defaultValue?: string | string[];
onValueChange?: (value: string | string[]) => void
}

const AccordionRoot = ({ children, orientation = 'vertical', asChild, transitionDuration = 0, transitionTimingFunction = 'linear', customRootClass, loop = true, openMultiple = false }: AccordionRootProps) => {
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);

Expand All @@ -35,7 +38,10 @@ const AccordionRoot = ({ children, orientation = 'vertical', asChild, transition
accordionRef,
transitionDuration,
transitionTimingFunction,
openMultiple
openMultiple,
value,
defaultValue,
onValueChange
Comment on lines +41 to +44
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Initialize state from defaultValue
  2. Update state when value changes
  3. Call onValueChange when 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.

Suggested change
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
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, implementation missing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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:

  1. Update the AccordionContext type definition to include the new props
  2. 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:

  1. Handles initialization from defaultValue
  2. Updates state when value changes (controlled mode)
  3. Properly calls onValueChange when state changes
  4. 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 AccordionContext using React's createContext. It initializes the context with default values for properties such as rootClass, activeItems, setActiveItems, accordionRef, transitionDuration, transitionTimingFunction, and openMultiple. This context is used in the AccordionRoot component 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_NAME for the Progress component. While it is not directly related to the AccordionRoot, 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.

}}>
<RovingFocusGroup.Root orientation={orientation} loop={loop}>
<RovingFocusGroup.Group >
Expand Down