[WEB-4689]chore: added accordion to propel#7641
Conversation
WalkthroughAdds a new Accordion module implemented atop BaseAccordion, introduces a barrel export for it, and updates package exports to expose the accordion entrypoint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Consumer App
participant Accordion as propel/Accordion
participant Base as BaseAccordion
participant DOM as Browser DOM
App->>Accordion: render <Accordion.Root defaultValue, allowMultiple, ...>
Accordion->>Base: render Root (openMultiple = allowMultiple, defaultValue)
loop per item
Accordion->>Base: render Item(value=id, disabled?)
Accordion->>Base: render Header -> Trigger (asChild?, icon)
Accordion->>Base: render Panel (content, --accordion-panel-height)
end
User->>DOM: click Trigger
DOM->>Base: propagate event to toggle item
Base-->>DOM: update data-state -> animate panel (open/close)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/propel/src/accordion/accordion.tsx (7)
1-3: Reorder imports to satisfy import/orderESLint expects the React import first. Reordering also matches common conventions.
Apply this diff:
-import { Accordion as BaseAccordion } from "@base-ui-components/react/accordion"; -import { PlusIcon } from "lucide-react"; -import * as React from "react"; +import * as React from "react"; +import { Accordion as BaseAccordion } from "@base-ui-components/react/accordion"; +import { PlusIcon } from "lucide-react";
50-51: Tailwind z-index class is invalid; use arbitrary value or a scale tokenTailwind doesn’t provide z-1 by default. Use z-[1] (arbitrary) or a standard token like z-10.
The diff in the main fix already swaps to z-[1]. If you prefer a token:
- focus-visible:z-1 + focus-visible:z-10
31-33: Hide decorative icon from assistive techMark the default PlusIcon as aria-hidden, since it’s purely decorative.
This is included in the main diff. If you keep the current structure, minimally:
- icon = ( - <PlusIcon className="mr-2 size-3 shrink-0 transition-all ease-out group-data-[panel-open]:scale-110 group-data-[panel-open]:rotate-45" /> - ), + icon = ( + <PlusIcon + aria-hidden="true" + className="mr-2 size-3 shrink-0 transition-all ease-out group-data-[panel-open]:scale-110 group-data-[panel-open]:rotate-45" + /> + ),
12-21: Consider widening types and exposing Root props for flexibility
- Base UI’s Item value type is AccordionItemValue (not strictly string). If you anticipate numeric IDs, widen id to string | number to match expectations.
- Consider extending BaseAccordion.Root props (value, onValueChange, orientation, keepMounted, etc.) so consumers can opt into controlled mode and advanced behavior.
Proposed type changes:
-export interface AccordionItem { - id: string; +export interface AccordionItem { + id: string | number; title: React.ReactNode; content: React.ReactNode; disabled?: boolean; } -export interface AccordionProps { +export interface AccordionProps + extends Omit< + React.ComponentPropsWithoutRef<typeof BaseAccordion.Root>, + "className" | "defaultValue" | "openMultiple" + > { items: AccordionItem[]; allowMultiple?: boolean; defaultValue?: string[]; className?: string; itemClassName?: string; triggerClassName?: string; panelClassName?: string; icon?: React.ReactNode; }Note: If you adopt this, plumb value/onValueChange/orientation through to Root.
(base-ui.com)
49-54: Optional: simplify “open” icon behavior using selector-based variantYou can avoid relying on group variants and instead style the icon based on the Trigger’s own data attribute, e.g. “[&[data-panel-open]>svg]:rotate-180”, a pattern seen in community wrappers around Base UI. Functionally equivalent and slightly more robust to DOM changes. (kopi-ui.com)
Example:
-<BaseAccordion.Trigger - className={`group ... ${triggerClassName}`} -> - {item.title} - {icon} -</BaseAccordion.Trigger> +<BaseAccordion.Trigger + className={`...[&[data-panel-open]>svg]:rotate-45 ${triggerClassName}`} +> + {item.title} + <span aria-hidden="true">{icon}</span> +</BaseAccordion.Trigger>
23-23: Optional: forward refs to RootForwarding a ref makes this wrapper drop-in for cases where consumers need to measure or scroll to the accordion.
Refactor:
-export const Accordion: React.FC<AccordionProps> = (props) => ( ... ); +export const Accordion = React.forwardRef<HTMLDivElement, AccordionProps>( + ( + { + items, + allowMultiple = false, + defaultValue = [], + className = "", + itemClassName = "", + triggerClassName = "", + panelClassName = "", + icon = <PlusIcon aria-hidden="true" className="mr-2 size-3 shrink-0 transition-all ease-out group-data-[panel-open]:scale-110 group-data-[panel-open]:rotate-45" />, + ...rootProps + }, + ref + ) => ( + <BaseAccordion.Root + ref={ref} + defaultValue={defaultValue} + openMultiple={allowMultiple} + className={`flex flex-col justify-center text-gray-900 ${className}`} + {...rootProps} + > + {/* ... */} + </BaseAccordion.Root> + ) +);
41-62: General UX: titles as ReactNode are flexible — document expected semanticsAllowing ReactNode for title and content is great for flexibility. Please document that titles should be concise and that ids must be unique across items to prevent React key collisions and accordion value collisions.
Would you like me to add a short JSDoc block above the props with these notes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/accordion/accordion.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build and lint web apps
packages/propel/src/accordion/accordion.tsx
[failure] 34-34:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/accordion/accordion.tsx
[warning] 3-3: ESLint: import/order - 'react' import should occur before import of '@base-ui-components/react/accordion'.
[error] 34-34: ESLint: arrow-body-style - Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/propel/src/accordion/accordion.tsx (2)
36-40: API check: props align with Base UI (openMultiple, defaultValue) — confirm intentional default
- Base UI’s Accordion.Root supports openMultiple and uses an array for defaultValue and value. Your usage aligns with the API. Note that Base UI defaults openMultiple to true; you default allowMultiple to false. Confirm this behavior change is intentional across consumers. (base-ui.com)
If this was unintentional, change allowMultiple’s default to true to mirror the underlying component.
41-62: State-driven styling is compatible with Base UIUsing data attributes for styling is correct here:
- Trigger: data-panel-open
- Panel: data-starting-style / data-ending-style
- CSS var: --accordion-panel-height
This matches Base UI’s exposed attributes and variables. (base-ui.com)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/propel/package.json (1)
26-27: Optional: keep exports alphabetized for quicker scansIf you keep this object alphabetized, it’s easier to spot duplicates and find modules. Consider moving
"./accordion"near the top alongside other A-entries. No functional impact; purely organizational.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/propel/package.json(1 hunks)packages/propel/src/accordion/accordion.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/accordion/accordion.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/propel/package.json (1)
26-27: Accordion export wired up correctlyThe new subpath export for the Accordion module looks correct and consistent with existing patterns. The comma after the preceding entry is appropriate (JSON requires it since another key follows). This should enable
import { Accordion } from "@plane/propel/accordion"for consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/propel/src/accordion/index.ts (1)
1-1: Prefer explicit and type-only re-exports to avoid leaking internals.Wildcard re-exports can unintentionally expose non-public symbols from
./accordion. Enumerating value exports and usingexport typefor types keeps the API surface intentional and tree-shaker friendly.Proposed change (adjust names if they differ):
-export * from "./accordion"; +export { Accordion, AccordionItem } from "./accordion"; +export type { AccordionProps } from "./accordion";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/propel/src/accordion/accordion.tsx(1 hunks)packages/propel/src/accordion/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/accordion/accordion.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
packages/propel/src/accordion/index.ts (1)
1-1: LGTM: Barrel export set up for Accordion public API.This establishes a clean entrypoint for consumers via
propel/accordion.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/propel/src/accordion/accordion.tsx (3)
69-79: Optional: expose region semantics via content wrapper (only if BaseAccordion.Panel doesn’t already)If BaseAccordion.Panel doesn’t set appropriate roles/aria, consider ensuring the panel is announced as a region and linked to its trigger via aria-labelledby. Otherwise, ignore this.
Example (only if needed, not a hard requirement):
- <BaseAccordion.Panel + <BaseAccordion.Panel + role="region" + aria-labelledby={/* id of the corresponding trigger */ undefined} className={`h-[var(--accordion-panel-height)] overflow-hidden transition-[height] ease-out data-[ending-style]:h-0 data-[starting-style]:h-0 ${className}`} >
81-86: Polish: add displayName for better DevTools labels and re-export prop typesThis improves DX without affecting runtime.
@@ export const Accordion = { Root: AccordionRoot, Item: AccordionItem, Trigger: AccordionTrigger, Content: AccordionContent, }; + +// Better labels in React DevTools +AccordionRoot.displayName = "PropelAccordionRoot"; +AccordionItem.displayName = "PropelAccordionItem"; +AccordionTrigger.displayName = "PropelAccordionTrigger"; +AccordionContent.displayName = "PropelAccordionContent"; + +// Re-export prop types for consumers +export type { + AccordionRootProps, + AccordionItemProps, + AccordionTriggerProps, + AccordionContentProps, +};
33-48: Optional: forward refs through to underlying Base componentsForwarding refs on Root/Item/Trigger/Content helps consumers integrate with focus management and measurements. If acceptable for your API, I can provide a precise refactor using React.forwardRef for each component.
Would you like me to follow up with a forwardRef diff for all four components?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/accordion/accordion.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
3 participants
Description
In this update a
Accordionis added to propel package.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Chores