[WEB-4688]chore: added collapsible to propel#7643
Conversation
WalkthroughAdds a new Collapsible compound component (Root/Trigger/Content) under packages/propel/src/collapsible, wires it into the build (tsdown) and public exports (package.json), and creates a barrel index for re-exports. No changes to existing modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant T as Collapsible.Trigger
participant Ctx as CollapsibleContext
participant R as Collapsible.Root
participant Base as BaseCollapsible
U->>T: Click
T->>Ctx: onToggle()
alt Uncontrolled
Ctx->>R: toggle local isOpen
else Controlled
Ctx->>R: call onToggle (delegates to parent)
end
R->>Base: open / onOpenChange
Note over Base,R: Base primitives render Panel visibility accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
|
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: 4
🧹 Nitpick comments (4)
packages/propel/src/collapsible/collapsible.tsx (4)
19-23: Forward ref and pass through native props on Trigger.Use forwardRef and widen props to the underlying trigger for compatibility and a11y extensions.
-type TriggerProps = { - children: React.ReactNode; - className?: string; - buttonRef?: React.RefObject<HTMLButtonElement>; -}; +type TriggerProps = React.ComponentPropsWithoutRef<typeof BaseCollapsible.Trigger>; @@ -const Trigger: React.FC<TriggerProps> = ({ children, className, buttonRef }) => { +const Trigger = React.forwardRef<HTMLButtonElement, TriggerProps>(({ children, className, ...props }, ref) => { const { isOpen } = useCollapsible(); - return ( - <BaseCollapsible.Trigger data-panel-open={isOpen} ref={buttonRef} className={className}> - {children} - </BaseCollapsible.Trigger> - ); -}; + return ( + <BaseCollapsible.Trigger + data-panel-open={isOpen} + ref={ref} + className={className} + {...props} + > + {children} + </BaseCollapsible.Trigger> + ); +});Also update imports to include forwardRef and ComponentPropsWithoutRef (see a separate comment).
Also applies to: 74-82
25-28: Forward ref and pass through native props on Content.Align with Trigger for consistency and flexibility.
-type ContentProps = { - children: React.ReactNode; - className?: string; -}; +type ContentProps = React.ComponentPropsWithoutRef<typeof BaseCollapsible.Panel>; @@ -const Content: React.FC<ContentProps> = ({ children, className }) => { - return ( - <BaseCollapsible.Panel +const Content = React.forwardRef<HTMLDivElement, ContentProps>(({ children, className, ...props }, ref) => { + return ( + <BaseCollapsible.Panel + ref={ref} className={clsx( "flex h-[var(--collapsible-panel-height)] flex-col overflow-hidden text-sm transition-all ease-out data-[ending-style]:h-0 data-[starting-style]:h-0", className )} + {...props} > {children} </BaseCollapsible.Panel> - ); -}; + ); +});Also applies to: 84-95
1-1: Add missing imports for forwardRef and widened prop types.Required by the previous two refactors.
-import React, { useState, useEffect, useCallback, createContext, useContext } from "react"; +import React, { useState, useEffect, useCallback, createContext, useContext, forwardRef } from "react"; +import type { ComponentPropsWithoutRef } from "react";
98-102: Add short aliases: Collapsible.Root/Trigger/Content.Keeps API familiar while preserving current names.
export const Collapsible = { - CollapsibleRoot: Root, - CollapsibleTrigger: Trigger, - CollapsibleContent: Content, + // Short aliases + Root: Root, + Trigger: Trigger, + Content: Content, + // Existing names + CollapsibleRoot: Root, + CollapsibleTrigger: Trigger, + CollapsibleContent: Content, };
📜 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 (3)
packages/propel/package.json(1 hunks)packages/propel/src/collapsible/collapsible.tsx(1 hunks)packages/propel/src/collapsible/index.ts(1 hunks)
🔇 Additional comments (2)
packages/propel/src/collapsible/collapsible.tsx (1)
52-58: Use nextOpen from onOpenChange to avoid state desyncDerive state from the callback argument instead of blindly inverting.
Confirm BaseCollapsible.Root’s onOpenChange signature (does it pass nextOpen: boolean or (event, data)?), then map accordingly:
- const handleToggle = useCallback(() => { - if (controlledIsOpen !== undefined) { - onToggle?.(); - } else { - setLocalIsOpen((prev) => !prev); - } - }, [controlledIsOpen, onToggle]); + const handleToggle = useCallback((nextOpen?: boolean) => { + if (controlledIsOpen !== undefined) { + onToggle?.(nextOpen ?? !controlledIsOpen); + } else { + setLocalIsOpen((prev) => (nextOpen ?? !prev)); + } + }, [controlledIsOpen, onToggle]);Also apply the same change at line 66.
packages/propel/src/collapsible/index.ts (1)
1-1: LGTM: Barrel re-export is correct.Keeps the public surface tidy and consistent with other modules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/propel/package.json (2)
28-30: Verify the new export resolves correctly.Confirm the export maps and source barrel exist to prevent runtime resolution issues.
#!/bin/bash # 1) Check export mapping jq -r '.exports["./collapsible"]' packages/propel/package.json # 2) Ensure the barrel file exists fd -p 'src/collapsible/index.ts' packages/propel # 3) Ensure barrel re-exports the component(s) rg -n 'export \* from "./collapsible";' packages/propel/src/collapsible/index.ts -C0
28-30: Document usage in README/Storybook.Add a minimal import/usage snippet and a basic Storybook story for Collapsible so consumers see API and import path.
Happy to draft the README snippet and a Storybook story if you want.
📜 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/package.json(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/propel/package.json
[error] 30-30: expected , but instead found "./card"
Remove "./card"
(parse)
🪛 GitHub Actions: Build and lint web apps
packages/propel/package.json
[error] 30-30: Command failed: pnpm install --frozen-lockfile. PNPM JSON parse error in '/home/runner/work/plane/plane/packages/propel/package.json': ERR_PNPM_JSON_PARSE: Expected ',' or '}' after property value in JSON at position 1125 (line 30 column 5).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/propel/package.json (2)
28-30: New public export looks correct.The mapping to "./src/collapsible/index.ts" is valid and the JSON is well-formed.
16-31: Add README/Storybook usage snippet for Collapsible (import path).Document a minimal example in Propel README and add a Storybook story for discoverability.
packages/propel/src/collapsible/collapsible.tsx (3)
6-9: Pass next state through callbacks (types and API).Expose the next open state for controlled usage and parity with Base UI’s onOpenChange.
Apply:
type CollapsibleContextType = { isOpen: boolean; - onToggle: () => void; + onToggle: (nextOpen: boolean) => void; }; type RootProps = { children: React.ReactNode; className?: string; isOpen?: boolean; - onToggle?: () => void; + onToggle?: (nextOpen: boolean) => void; defaultOpen?: boolean; };Also applies to: 11-17
44-44: Fix initial state: false incorrectly falls back to defaultOpen.Use nullish coalescing to preserve explicit false.
- const [localIsOpen, setLocalIsOpen] = useState<boolean>(controlledIsOpen || defaultOpen || false); + const [localIsOpen, setLocalIsOpen] = useState<boolean>(controlledIsOpen ?? defaultOpen ?? false);
52-58: onOpenChange handler ignores nextOpen; gate defaultOpen when controlled.Current handler toggles blindly and passes a no-arg function to Base UI. Use the provided nextOpen boolean and avoid mixing defaultOpen with controlled mode.
- const handleToggle = useCallback(() => { - if (controlledIsOpen !== undefined) { - onToggle?.(); - } else { - setLocalIsOpen((prev) => !prev); - } - }, [controlledIsOpen, onToggle]); + const handleOpenChange = useCallback( + (nextOpen: boolean) => { + if (controlledIsOpen === undefined) { + setLocalIsOpen(nextOpen); + } + onToggle?.(nextOpen); + }, + [controlledIsOpen, onToggle] + ); return ( - <CollapsibleContext.Provider value={{ isOpen: localIsOpen, onToggle: handleToggle }}> + <CollapsibleContext.Provider value={{ isOpen: localIsOpen, onToggle: handleOpenChange }}> <BaseCollapsible.Root className={clsx(className)} - defaultOpen={defaultOpen} + {...(controlledIsOpen === undefined ? { defaultOpen } : {})} open={localIsOpen} - onOpenChange={handleToggle} + onOpenChange={handleOpenChange} >Also applies to: 60-67
🧹 Nitpick comments (3)
packages/propel/src/collapsible/collapsible.tsx (3)
19-23: Forward refs and pass-through props on Trigger.Improve ergonomics and interop by using forwardRef and allowing standard button/trigger props.
-type TriggerProps = { - children: React.ReactNode; - className?: string; - buttonRef?: React.RefObject<HTMLButtonElement>; -}; +type TriggerProps = React.ComponentPropsWithoutRef<typeof BaseCollapsible.Trigger>; -const Trigger: React.FC<TriggerProps> = ({ children, className, buttonRef }) => { - const { isOpen } = useCollapsible(); - - return ( - <BaseCollapsible.Trigger data-panel-open={isOpen} ref={buttonRef} className={className}> - {children} - </BaseCollapsible.Trigger> - ); -}; +const Trigger = React.forwardRef<HTMLButtonElement, TriggerProps>(({ className, ...props }, ref) => { + const { isOpen } = useCollapsible(); + return ( + <BaseCollapsible.Trigger + data-panel-open={isOpen} + ref={ref} + className={clsx(className)} + {...props} + /> + ); +}); +Trigger.displayName = "CollapsibleTrigger";Also applies to: 74-82
25-29: Forward refs and pass-through props on Content.Allow standard div/panel props and external refs.
-type ContentProps = { - children: React.ReactNode; - className?: string; -}; +type ContentProps = React.ComponentPropsWithoutRef<typeof BaseCollapsible.Panel>; -const Content: React.FC<ContentProps> = ({ children, className }) => ( - <BaseCollapsible.Panel +const Content = React.forwardRef<HTMLDivElement, ContentProps>(({ className, ...props }, ref) => ( + <BaseCollapsible.Panel + ref={ref} className={clsx( "flex h-[var(--collapsible-panel-height)] flex-col overflow-hidden text-sm transition-all ease-out data-[ending-style]:h-0 data-[starting-style]:h-0", className )} + {...props} > - {children} + {props.children} </BaseCollapsible.Panel> -); +)); +Content.displayName = "CollapsibleContent";Also applies to: 84-93
95-100: Export prop types and freeze compound object.Expose types for consumers and mark the compound export as readonly.
-export const Collapsible = { +export const Collapsible = { CollapsibleRoot: Root, CollapsibleTrigger: Trigger, CollapsibleContent: Content, -}; +} as const; + +export type { + RootProps as CollapsibleRootProps, + TriggerProps as CollapsibleTriggerProps, + ContentProps as CollapsibleContentProps, +};
📜 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/collapsible/collapsible.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)
🔇 Additional comments (2)
packages/propel/package.json (1)
28-30: Exports for “./collapsible” are correctly configured; consumers can import from "@plane/propel/collapsible".packages/propel/src/collapsible/collapsible.tsx (1)
86-89: No changes needed: data-starting-style and data-ending-style match Base UI
…psible_migration_propel
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/propel/src/collapsible/collapsible.tsx (3)
6-9: Make onToggle convey next state; wire through onOpenChange.Controlled consumers need the next open value. Also, BaseCollapsible’s onOpenChange provides the next state; use it instead of toggling locally.
Apply:
type CollapsibleContextType = { isOpen: boolean; - onToggle: () => void; + onToggle: (nextOpen: boolean) => void; }; type RootProps = { children: React.ReactNode; className?: string; isOpen?: boolean; - onToggle?: () => void; + onToggle?: (nextOpen: boolean) => void; defaultOpen?: boolean; }; - const handleToggle = useCallback(() => { - if (controlledIsOpen !== undefined) { - onToggle?.(); - } else { - setLocalIsOpen((prev) => !prev); - } - }, [controlledIsOpen, onToggle]); + const handleToggle = useCallback( + (nextOpen: boolean) => { + if (controlledIsOpen !== undefined) { + onToggle?.(nextOpen); + } else { + setLocalIsOpen(nextOpen); + } + }, + [controlledIsOpen, onToggle] + );To confirm the callback shape, please check the latest API:
What is the onOpenChange callback signature for @base-ui-components/react/collapsible (v1.0.0-beta.2)? Does it pass the next open boolean?Also applies to: 11-17, 52-58
44-44: Fix initial-state flicker (false treated as fallback).Preserve explicit false via nullish coalescing.
- const [localIsOpen, setLocalIsOpen] = useState<boolean>(controlledIsOpen || defaultOpen || false); + const [localIsOpen, setLocalIsOpen] = useState<boolean>(controlledIsOpen ?? defaultOpen ?? false);
64-66: Avoid mixing controlled and uncontrolled props.Gate defaultOpen when controlled to prevent confusing semantics.
- defaultOpen={defaultOpen} + defaultOpen={controlledIsOpen === undefined ? defaultOpen : undefined} open={localIsOpen} - onOpenChange={handleToggle} + onOpenChange={handleToggle}packages/propel/package.json (1)
23-23: Expose usage in docs/Storybook for the new subpath.Add a brief README and Storybook example showing:
import { Collapsible } from "@plane/propel/collapsible".
🧹 Nitpick comments (2)
packages/propel/src/collapsible/collapsible.tsx (2)
19-23: Prefer forwardRef over custom buttonRef prop.Forwarding refs is the conventional ergonomics for host elements.
-type TriggerProps = { - children: React.ReactNode; - className?: string; - buttonRef?: React.RefObject<HTMLButtonElement>; -}; +type TriggerProps = { + children: React.ReactNode; + className?: string; +}; -const Trigger: React.FC<TriggerProps> = ({ children, className, buttonRef }) => { +const Trigger = React.forwardRef<HTMLButtonElement, TriggerProps>(({ children, className }, ref) => { const { isOpen } = useCollapsible(); return ( - <BaseCollapsible.Trigger data-panel-open={isOpen} ref={buttonRef} className={className}> + <BaseCollapsible.Trigger data-panel-open={isOpen} ref={ref} className={className}> {children} </BaseCollapsible.Trigger> ); -}; +}); +Trigger.displayName = "CollapsibleTrigger";Also applies to: 74-82
6-9: Context surface: drop onToggle or export a hook if needed.onToggle in context isn’t consumed by Trigger/Content and useCollapsible isn’t exported; consider removing it from context to reduce API surface, or export the hook for custom triggers.
Also applies to: 30-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/propel/package.json(1 hunks)packages/propel/src/collapsible/collapsible.tsx(1 hunks)packages/propel/src/collapsible/index.ts(1 hunks)packages/propel/tsdown.config.ts(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)
🔇 Additional comments (2)
packages/propel/tsdown.config.ts (1)
9-9: LGTM: entry added and aligned with exports.packages/propel/src/collapsible/index.ts (1)
1-1: LGTM: barrel export is correct.
4 participants
Description
This update adds collapsible to propel.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Chores