feat: add collapsible support to PanelCard component#23366
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughChartCard was refactored to accept a single props object typed as PanelCardProps & { legend?: LegendItem[]; legendSize?: LegendSize } and now forwards those props (including children) to PanelCard. ChartCard composes headerContent from an optional headerContent plus the legend, or renders only the legend when headerContent is absent. PanelCard gained collapsible behavior with new props Possibly related PRs
✨ 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
Status, Documentation and Community
|
eunjae-lee
changed the base branch from
main
to
devin/toggleable-legend-1756304974
eunjae-lee
changed the base branch from
devin/toggleable-legend-1756304974
to
main
- Add collapsible and defaultCollapsed props to PanelCard - When collapsible is enabled, title becomes clickable with chevron icon - Chevron icon indicates collapse state and rotates on toggle - Body content is hidden when collapsed - Maintains backward compatibility with existing usage - Follows FormCard's collapsible pattern implementation Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> ^ Conflicts: ^ packages/ui/components/card/PanelCard.tsx
eunjae-lee
force-pushed
the
devin/collapsible-panel-card-1756225149
branch
from
6893c2e to
72db4f1
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/insights/components/ChartCard.tsx (1)
60-61: Ensure stable, unique React keys in Legend
key={item.label}risks collisions when labels repeat. Use a composite key.Apply this diff:
- <Fragment key={item.label}> + <Fragment key={`${item.label}-${index}`}>
🧹 Nitpick comments (5)
packages/features/insights/components/ChartCard.tsx (1)
56-59: Add basic a11y semantics to Legend (optional)Expose Legend to AT as a list and label it. Localize the label with
t()per guidelines.Apply this diff:
- return ( - <div className="bg-default flex items-center gap-2 rounded-lg px-1.5 py-1"> + return ( + <div + className="bg-default flex items-center gap-2 rounded-lg px-1.5 py-1" + role="list" + aria-label="Legend"> {items.map((item, index) => (- <div + <div + role="listitem" className="relative flex items-center gap-2 rounded-md px-1.5 py-0.5" style={{ backgroundColor: `${item.color}33` }}>Note: wrap "Legend" with
t()per project conventions.Also applies to: 62-63, 75-75
packages/ui/components/card/PanelCard.tsx (4)
51-53: Minor: data attribute for styling states (optional)Consider
data-collapsed={isCollapsed}on the root for easier state-based styling in CSS without extra conditional classes.
19-21: Controlled state hook (optional)If you foresee external control, consider adding
collapsed?: booleanandonCollapsedChange?(next: boolean)to support controlled/uncontrolled patterns without breaking current API.Also applies to: 29-31
97-103: Optional: performance escape hatchConsider
unmountOnCollapse?: booleanto unmount heavy children (e.g., charts) when collapsed. Default to current behavior (hide only).
55-91: Add unit and a11y tests for the collapsible behaviorTests to add:
- Renders with
collapsible=trueshows chevron and sets correctaria-expanded.- Clicking chevron/title toggles
aria-expandedand hides/shows content.- Content region has
role="region"and correctaria-controls/aria-labelledby.I can scaffold Jest/RTL tests covering these cases on request.
Also applies to: 92-103
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/features/insights/components/ChartCard.tsx(2 hunks)packages/ui/components/card/PanelCard.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/ui/components/card/PanelCard.tsxpackages/features/insights/components/ChartCard.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/card/PanelCard.tsxpackages/features/insights/components/ChartCard.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/ui/components/card/PanelCard.tsxpackages/features/insights/components/ChartCard.tsx
🧬 Code graph analysis (2)
packages/ui/components/card/PanelCard.tsx (1)
packages/ui/components/badge/index.ts (1)
InfoBadge(5-5)
packages/features/insights/components/ChartCard.tsx (1)
packages/ui/components/card/PanelCard.tsx (1)
PanelCard(11-106)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (5)
packages/features/insights/components/ChartCard.tsx (2)
9-10: Good: props typed via PanelCard’s inferred propsUsing
type PanelCardProps = React.ComponentProps<typeof PanelCard>future-proofs ChartCard against PanelCard API drift.
26-26: Optional: validate hex color inputs to avoid invalid CSSIf
itemscan be user-supplied, consider validatingitem.coloras#RRGGBBbefore appending alpha (33) to avoid rendering glitches.Do you want a small helper like
withAlphaHex(color: string, alphaHex: string)and unit tests?packages/ui/components/card/PanelCard.tsx (3)
86-90: Localize CTA label upstream
cta.labelis rendered as-is. Ensure callers pass a localized string viat(); no change needed here, just a reminder per guidelines.
1-2: LGTM: client-only component is appropriateMarking as
"use client"is correct since the component manages interactive state.
32-36: Hydration note
defaultCollapsedis read only on first render. Changes to the prop post-mount won’t sync state—document this or switch to a controlled prop if needed.
There was a problem hiding this comment.
I wonder if we can add the useAutoAnimate hook from formkit auto animate (we use this in loads of places)
To make this transition a bit smoother when toggling its a bit too snappy at the mooment.
Coderabbit suggestions here are pretty good.
There was a problem hiding this comment.
I wonder if we can add the useAutoAnimate hook from formkit auto animate (we use this in loads of places)
To make this transition a bit smoother when toggling its a bit too snappy at the mooment.
Coderabbit suggestions here are pretty good.
sean-brydon
left a comment
sean-brydon
left a comment
There was a problem hiding this comment.
Overall LGMT - i think we should address feedback + code rabbit before merging as theyre quick wins
dosubot
bot
added
insights
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/ui/components/card/PanelCard.tsx (1)
55-76: Collapsible + ARIA wiring looks solidGood adoption of WAI-ARIA pattern: separate chevron button and title button, synced aria-expanded/controls, region with aria-labelledby, and useAutoAnimate for transitions.
Also applies to: 78-99, 109-122
packages/features/insights/components/ChartCard.tsx (1)
30-41: Header content merge is correctYou now preserve consumer-provided headerContent and append the legend—avoids surprising overrides.
🧹 Nitpick comments (5)
packages/ui/components/card/PanelCard.tsx (4)
50-52: Fix spacing when collapsible=false but defaultCollapsed=truepb-1 is tied to isCollapsed, which can hide padding in non-collapsible mode if defaultCollapsed=true. Tie it to actual visibility instead.
Apply:
const isStringTitle = typeof title === "string"; + const isHidden = collapsible && isCollapsed;- "bg-muted group relative flex w-full flex-col items-center rounded-2xl px-1", - !isCollapsed && "pb-1", + "bg-muted group relative flex w-full flex-col items-center rounded-2xl px-1", + !isHidden && "pb-1",- {!(isCollapsed && collapsible) && ( + {!isHidden && (Also applies to: 109-110, 44-45
81-89: Add visible focus styles to title buttonKeyboard focus on the title toggle lacks a visible indicator. Add focus-visible rings for a11y parity with the chevron button.
- <button + <button type="button" onClick={toggleCollapse} - className="text-left transition-opacity hover:opacity-80" + className="text-left transition-opacity hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-brand focus-visible:ring-offset-2" aria-expanded={!isCollapsed} aria-controls={contentId} aria-label={isCollapsed ? t("expand_panel") : t("collapse_panel")}>
23-33: Keep region labeled when title is a ReactNodeWhen title isn’t a string, aria-labelledby is unset, leaving the region unlabeled. Provide an optional explicit label.
children: ReactNode; collapsible?: boolean; defaultCollapsed?: boolean; + regionLabel?: string; }) {- role="region" - aria-labelledby={isStringTitle ? titleId : undefined} + role="region" + aria-labelledby={isStringTitle ? titleId : undefined} + aria-label={!isStringTitle ? regionLabel : undefined}Also applies to: 111-114
103-105: Ensure Button doesn’t submit forms inadvertentlyIf Button defaults to type="submit", this could submit enclosing forms. Explicitly set type="button" unless your Button component already does.
- <Button className="shrink-0" color="secondary" onClick={cta.onClick}> + <Button type="button" className="shrink-0" color="secondary" onClick={cta.onClick}>packages/features/insights/components/ChartCard.tsx (1)
71-71: Stabilize keys in Legend to avoid collisionsUsing label alone can collide if labels repeat. Include index.
- {items.map((item, index) => ( - <Fragment key={item.label}> + {items.map((item, index) => ( + <Fragment key={`${item.label}-${index}`}>
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/insights/components/ChartCard.tsx(2 hunks)packages/ui/components/card/PanelCard.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/public/static/locales/en/common.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/insights/components/ChartCard.tsxpackages/ui/components/card/PanelCard.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/insights/components/ChartCard.tsxpackages/ui/components/card/PanelCard.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/insights/components/ChartCard.tsxpackages/ui/components/card/PanelCard.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-29T22:57:30.345Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/form-builder/FormBuilder.tsx:11-11
Timestamp: 2025-08-29T22:57:30.345Z
Learning: FormBuilder.tsx in packages/features/form-builder/ does not have "use client" directive at the top despite using client-side React hooks and event handlers, which suggests it should be a client component.
Applied to files:
packages/ui/components/card/PanelCard.tsx
🧬 Code graph analysis (1)
packages/features/insights/components/ChartCard.tsx (3)
packages/ui/components/card/PanelCard.tsx (1)
PanelCard(13-125)packages/features/insights/components/booking/EventTrendsChart.tsx (1)
legend(23-30)packages/features/insights/components/routing/RoutingFunnelContent.tsx (1)
legend(25-29)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
| titleTooltip?: string; | ||
| children: ReactNode; | ||
| }) { | ||
| const legendComponent = legend && legend.length > 0 ? <Legend items={legend} size={legendSize} /> : null; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify legend labels are localized at call sites
ChartCard correctly renders provided labels; ensure callers pass t()-wrapped labels to meet i18n guidelines.
🏁 Script executed:
#!/bin/bash
# Find legend definitions with hardcoded English labels (not t(...))
rg -nP --type=ts --type=tsx -C1 '(legend\s*=\s*\[|\blegend:\s*\[)' \
| rg -nP 'label:\s*"[^"]+"' -C0Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for literal label definitions in TSX files (potential hardcoded legend labels)
rg -nP 'label\s*:\s*"[^"]+"' -g '*.tsx'
# Find ChartCard usages with a legend prop and inspect how legend is passed
rg -nP -g '*.tsx' '<ChartCard\b' -C3 | rg -C3 'legend\s*='Length of output: 36017
🏁 Script executed:
#!/bin/bash
# Display the legend definition in EventTrendsChart.tsx
sed -n '1,200p' packages/features/insights/components/booking/EventTrendsChart.tsx
echo "---"
# Display the legend definition in RoutingFunnel.tsx
sed -n '1,200p' packages/features/insights/components/routing/RoutingFunnel.tsxLength of output: 4784
Localize legend labels in Insights components
- In packages/features/insights/components/booking/EventTrendsChart.tsx, the exported
legendarray uses hardcoded English strings ("Created","Completed", etc.); move its definition inside theEventTrendsChartcomponent (after callingconst { t } = useLocale()) and wrap each label int(). - In packages/features/insights/components/routing/RoutingFunnelContent.tsx, do the same for its exported
legendarray—translate each label viat()before passing toChartCard.
🤖 Prompt for AI Agents
In packages/features/insights/components/ChartCard.tsx around line 26, leave the
legend rendering as-is (it expects legend labels to already be localized) and
instead move the exported legend arrays from
packages/features/insights/components/booking/EventTrendsChart.tsx and
packages/features/insights/components/routing/RoutingFunnelContent.tsx into
their respective component functions (after const { t } = useLocale()), replace
the hardcoded English strings with t('...') calls for each label, and update any
imports/usages so ChartCard receives the already-translated legend prop (remove
or stop exporting the static legend arrays).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/insights/components/ChartCard.tsx (2)
37-49: Avoid extra wrapper when only one of headerContent or legend existsSmall DOM/paint reduction and simpler logic by wrapping only when both are present.
Apply this diff:
- headerContent={ - panelCardProps.headerContent ? ( - <div className="flex items-center gap-2"> - {panelCardProps.headerContent} - {legendComponent} - </div> - ) : ( - legendComponent - ) - }> + headerContent={ + panelCardProps.headerContent && legendComponent ? ( + <div className="flex items-center gap-2"> + {panelCardProps.headerContent} + {legendComponent} + </div> + ) : panelCardProps.headerContent ?? legendComponent + }>
104-105: Localize toggle button aria-label
In packages/features/insights/components/ChartCard.tsx’s Legend component:
- Add at the top with other imports:
import { useLocale } from "@calcom/lib/hooks/useLocale";- Inside Legend(), before the return, add:
const { t } = useLocale();- Replace
witharia-label={`Toggle ${item.label}`}aria-label={t("toggle_series", { label: item.label })}
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/features/insights/components/ChartCard.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/insights/components/ChartCard.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/insights/components/ChartCard.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/insights/components/ChartCard.tsx
🧬 Code graph analysis (1)
packages/features/insights/components/ChartCard.tsx (1)
packages/ui/components/card/PanelCard.tsx (1)
PanelCard(13-125)
🔇 Additional comments (2)
packages/features/insights/components/ChartCard.tsx (2)
9-10: LGTM: Prop alias keeps types in sync with PanelCardUsing React.ComponentProps to derive PanelCardProps avoids drift and improves maintainability.
23-24: LGTM: Rest capture preserves PanelCard API and backward compatibilityForwarding the remaining props via ...panelCardProps keeps ChartCard thin and non-breaking.
E2E results are ready! |
3 participants
What does this PR do?
This PR adds collapsible support to the newly extracted
PanelCardcomponent (stacked on top of PR #23360). The implementation allows cards to be collapsed/expanded with a chevron icon and clickable title, while maintaining full backward compatibility with existing usage.Key Features:
collapsible?: boolean- enables collapsible functionalitydefaultCollapsed?: boolean- sets initial collapse stateImplementation Details:
FormCarduseStatefor state managementButton,Icon)"use client") for interactivityVisual Demo
Screenshot.2025-08-28.at.16.53.26.mp4
I created a comprehensive test demonstrating the three usage scenarios:
The test shows:
How should this be tested?
Manual Testing:
/insights) where ChartCard components use PanelCardAccessibility Testing:
Environment:
Mandatory Tasks
Important Review Points
🔍 Critical areas to review:
useStateimplementation is correct for SSR/client rendering🧪 Testing gaps:
Session Info: Requested by @eunjae-lee
Link to Devin run: https://app.devin.ai/sessions/eb0405b25aa24df4879c736c0223d611