feat: add toggleable legend functionality to ChartCard components#23390
feat: add toggleable legend functionality to ChartCard components#23390eunjae-lee merged 8 commits intomainfrom
Conversation
- Create useToggleableLegend hook for managing enabled/disabled series - Update ChartCard to accept enabledLegend and onSeriesToggle props - Add click handlers and visual feedback to Legend component - Apply toggleable functionality to EventTrendsChart and RoutingFunnel - Maintain backwards compatibility with existing charts Features: - Visual feedback: 50% opacity for disabled items, hover effects, cursor pointer - Interactive legend items toggle chart series on/off - Hook uses TypeScript generics for different legend item types - Conditional rendering of chart series based on enabled legend items - Full backwards compatibility - existing charts work unchanged Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 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 adds optional 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
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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/routing/RoutingFunnelContent.tsx (1)
98-110: Tooltip percentages break when “Total Submissions” is toggled off.You read totalSubmissions from the tooltip payload, which omits hidden series, yielding 0% for others. Read from the datum instead.
- const totalSubmissions = payload.find((p) => p.dataKey === "totalSubmissions")?.value || 0; + const totalSubmissions = payload[0]?.payload?.totalSubmissions ?? 0;
🧹 Nitpick comments (5)
packages/features/insights/hooks/useToggleableLegend.ts (2)
8-11: Minor perf: avoid O(n^2) includes by using a Set.For larger legends, precompute a Set of enabled labels.
- const enabledLegend = useMemo( - () => legend.filter((item) => enabledSeries.includes(item.label)), - [legend, enabledSeries] - ); + const enabledLegend = useMemo(() => { + const set = new Set(enabledSeries); + return legend.filter((item) => set.has(item.label)); + }, [legend, enabledSeries]);
3-6: Optional: sync when legend changes.If legend is dynamic (locale/data), consider resetting enabled state when items change to keep UX predictable.
+ // Optional: re-enable all when legend changes + // useEffect(() => setEnabledSeries(initialEnabled ?? legend.map((i) => i.label)), [legend]);packages/features/insights/components/routing/RoutingFunnelContent.tsx (2)
32-35: Reduce repeated scans when checking active areas.You call some(...) three times per render. Precompute a Set for O(1) membership.
-export function RoutingFunnelContent({ data, enabledLegend }: RoutingFunnelContentProps) { +export function RoutingFunnelContent({ data, enabledLegend }: RoutingFunnelContentProps) { const { t } = useLocale(); - const activeAreas = enabledLegend || legend; + const activeAreas = enabledLegend || legend; + const active = new Set(activeAreas.map((a) => a.label)); ... -{activeAreas.some((area) => area.label === "Total Submissions") && ( +{active.has("Total Submissions") && ( ... -{activeAreas.some((area) => area.label === "Successful Routings") && ( +{active.has("Successful Routings") && ( ... -{activeAreas.some((area) => area.label === "Accepted Bookings") && ( +{active.has("Accepted Bookings") && (Also applies to: 43-52, 53-62, 63-72
21-24: Nit: fix “SUCCESFUL” spelling.Rename COLOR.SUCCESFUL to COLOR.SUCCESSFUL and update references for clarity.
- SUCCESFUL: "#89CFB5", + SUCCESSFUL: "#89CFB5",And replace COLOR.SUCCESFUL usages accordingly.
packages/features/insights/components/ChartCard.tsx (1)
84-96: Optional: avoid tinted bg overriding hover color.Combining item tint with hover:bg-gray-100 can produce muddy colors. Consider using ring/outline on hover instead.
📜 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 (5)
packages/features/insights/components/ChartCard.tsx(2 hunks)packages/features/insights/components/booking/EventTrendsChart.tsx(4 hunks)packages/features/insights/components/routing/RoutingFunnel.tsx(2 hunks)packages/features/insights/components/routing/RoutingFunnelContent.tsx(3 hunks)packages/features/insights/hooks/useToggleableLegend.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/routing/RoutingFunnelContent.tsxpackages/features/insights/hooks/useToggleableLegend.tspackages/features/insights/components/routing/RoutingFunnel.tsxpackages/features/insights/components/booking/EventTrendsChart.tsxpackages/features/insights/components/ChartCard.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-15T12:58:40.539Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
Applied to files:
packages/features/insights/components/routing/RoutingFunnelContent.tsx
🔇 Additional comments (7)
packages/features/insights/components/booking/EventTrendsChart.tsx (3)
70-71: Good hook integration.Using useToggleableLegend keeps the component lean and the state colocated with the legend definition.
88-94: Prop wiring looks correct and backward compatible.Passing legend, enabledLegend, and onSeriesToggle aligns with the updated ChartCard API.
107-119: Verify dataKey-to-label alignment.Lines render by dataKey=item.label. Ensure eventTrends objects have properties exactly matching labels (including spaces/case).
Run:
packages/features/insights/components/ChartCard.tsx (1)
16-24: API extension reads clean.New props enabledLegend and onSeriesToggle are optional and do not break existing usage.
Also applies to: 29-33, 35-39
packages/features/insights/components/routing/RoutingFunnel.tsx (3)
15-16: Hook adoption is consistent with EventTrends.Single source of truth for legend state improves UX across charts.
28-33: Nice: keep legend interactive even during skeleton.Maintains continuity of user toggles while data loads.
Also applies to: 39-45
39-45: Legend labels mapping verified
Thelegendlabels in RoutingFunnel.tsx (“Total Submissions”, “Successful Routings”, “Accepted Bookings”) exactly match the strings used in theactiveAreas.some(…).label === …checks in RoutingFunnelContent.tsx. No changes required.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
insights
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/features/insights/components/ChartCard.tsx (1)
93-123: Minor perf: avoid O(n·m) lookups on each render.Using a Set of enabled labels (as above) removes per-item linear scans. This matters if legends grow or re-render frequently.
📜 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 (2)
packages/features/insights/components/booking/EventTrendsChart.tsx (1)
legend(24-31)packages/features/insights/components/routing/RoutingFunnelContent.tsx (1)
legend(26-30)
⏰ 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 (3)
packages/features/insights/components/ChartCard.tsx (3)
22-24: New optional props are backward compatible — LGTM.
enabledLegend?andonSeriesToggle?extend the API without breaking existing consumers. Types are clear and specific.Also applies to: 33-35
39-43: Legend injection logic looks good.Conditional render keeps header clean when no legend is provided; props are forwarded correctly.
78-88: Legend component API is sound.Clear separation of concerns; sensible default for
size.
| const isClickable = !!onItemToggle; | ||
|
|
||
| return ( | ||
| <div className="bg-default flex items-center gap-2 rounded-lg px-1.5 py-1"> | ||
| {items.map((item, index) => ( | ||
| <Fragment key={item.label}> | ||
| <div | ||
| className="relative flex items-center gap-2 rounded-md px-1.5 py-0.5" | ||
| style={{ backgroundColor: `${item.color}33` }}> | ||
| <div className="h-2 w-2 rounded-full" style={{ backgroundColor: item.color }} /> | ||
| <Tooltip content={item.label}> | ||
| <p | ||
| className={classNames( | ||
| "text-default truncate py-0.5 text-sm font-medium leading-none", | ||
| size === "sm" ? "w-16" : "" | ||
| )}> | ||
| {item.label} | ||
| </p> | ||
| </Tooltip> | ||
| </div> | ||
| {index < items.length - 1 && <div className="bg-muted h-5 w-[1px]" />} | ||
| </Fragment> | ||
| ))} | ||
| {items.map((item, index) => { | ||
| const isEnabled = enabledItems ? enabledItems.some((enabled) => enabled.label === item.label) : true; | ||
|
|
||
| return ( | ||
| <Fragment key={item.label}> | ||
| <button | ||
| type="button" | ||
| className={classNames( | ||
| "relative flex items-center gap-2 rounded-md px-1.5 py-0.5 transition-opacity", | ||
| isClickable && "cursor-pointer hover:bg-gray-100", | ||
| !isEnabled && "opacity-25" | ||
| )} | ||
| style={{ backgroundColor: `${item.color}33` }} | ||
| aria-pressed={isEnabled} | ||
| aria-label={`Toggle ${item.label}`} | ||
| onClick={isClickable ? () => onItemToggle(item.label) : undefined}> | ||
| <div className="h-2 w-2 rounded-full" style={{ backgroundColor: item.color }} /> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Make non-clickable items truly non-interactive, localize aria-label, and align opacity with spec.
- Add
disabled={!isClickable}so non-interactive legends aren’t tabbable/clickable. - Only set
aria-pressedwhen interactive; otherwise omit it. - Replace hardcoded “Toggle …” with a localized string per repo guidelines.
- Product spec says disabled items should be 50% opacity; current code uses 25%.
Apply:
- const isClickable = !!onItemToggle;
+ const enabledSet = useMemo(() => new Set((enabledItems ?? []).map((i) => i.label)), [enabledItems]);
+ const isClickable = Boolean(onItemToggle);
@@
- const isEnabled = enabledItems ? enabledItems.some((enabled) => enabled.label === item.label) : true;
+ const isEnabled = enabledItems ? enabledSet.has(item.label) : true;
@@
<button
type="button"
className={classNames(
"relative flex items-center gap-2 rounded-md px-1.5 py-0.5 transition-opacity",
isClickable && "cursor-pointer hover:bg-gray-100",
- !isEnabled && "opacity-25"
+ !isEnabled && "opacity-50"
)}
style={{ backgroundColor: `${item.color}33` }}
- aria-pressed={isEnabled}
- aria-label={`Toggle ${item.label}`}
+ aria-pressed={isClickable ? isEnabled : undefined}
+ aria-label={t("insights.legend.toggle", { label: item.label })}
+ disabled={!isClickable}
onClick={isClickable ? () => onItemToggle(item.label) : undefined}>Additionally, add the necessary imports/initialization for your i18n utility and useMemo (example shown; adjust to your stack):
// at top
import { Fragment, useMemo, type ReactNode } from "react";
// inside Legend()
const t = useTranslations("insights");Make legend buttons non-interactive and accessible per spec
- Add
disabled={!isClickable}so buttons without anonItemTogglehandler aren’t tabbable/clickable - Only set
aria-pressedwhenisClickable; omit it otherwise - Localize the label: replace
aria-label={`Toggle ${item.label}`}witharia-label={t("insights.legend.toggle", { label: item.label })} - Use
opacity-50(50%) instead ofopacity-25for disabled items per product spec - Replace the repeated
.some()lookup with auseMemo-backedSetfor O(1) membership checks - Don’t forget to import
useMemoand initialize your translations hook (e.g.const t = useTranslations("insights"))
There was a problem hiding this comment.
@eunjae-lee Thank you for addressing the accessibility and performance improvements! Great work on implementing the disabled state, proper ARIA attributes, localized labels, correct opacity values, and the performance optimization with useMemo.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| className={classNames( | ||
| "relative flex items-center gap-2 rounded-md px-1.5 py-0.5 transition-opacity", | ||
| isClickable && "cursor-pointer hover:bg-gray-100", | ||
| !isEnabled && "opacity-25" |
There was a problem hiding this comment.
Interactive Legend component: Click handlers, 50% opacity for disabled items, hover effects
NIT: Do we want this 50 or is 25 okay?
There was a problem hiding this comment.
ah yeah actually 25 looked better
| const toggleSeries = useCallback( | ||
| (label: string) => { | ||
| setEnabledSeries((prev) => (prev.includes(label) ? prev.filter((s) => s !== label) : [...prev, label])); | ||
| }, | ||
| [setEnabledSeries] | ||
| ); |
There was a problem hiding this comment.
| const toggleSeries = useCallback( | |
| (label: string) => { | |
| setEnabledSeries((prev) => (prev.includes(label) ? prev.filter((s) => s !== label) : [...prev, label])); | |
| }, | |
| [setEnabledSeries] | |
| ); | |
| const toggleSeries = useCallback((label: string) => { | |
| setEnabledSeries((prev) => { | |
| const newSeries = prev.includes(label) | |
| ? prev.filter((s) => s !== label) | |
| : [...prev, label]; | |
| // Ensure at least one series remains visible | |
| return newSeries.length > 0 ? newSeries : prev; | |
| }); | |
| }, []); |
Does it make sense to ensure one of the items remains visible? Kinda weird we can deselect all legends
There was a problem hiding this comment.
I think practically it doesn't make any sense to show nothing but also it might be annoying if we don't allow users to deselect the last item (imagine this person deselects them all, and is going to start select something very soon)
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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/features/insights/components/ChartCard.tsx (1)
107-110: Localize or drop redundant aria-label.Per repo i18n rule, avoid hardcoded text; also the visible label already names the control, so aria-label may be unnecessary.
Option A — localize:
- aria-label={`Toggle ${item.label}`} + aria-label={t("insights.legend.toggle", { label: item.label })}(Add/import t() per your i18n setup.)
Option B — remove to rely on visible text:
- aria-label={`Toggle ${item.label}`}
🧹 Nitpick comments (2)
packages/features/insights/components/ChartCard.tsx (2)
22-24: Props shape looks good; tighten types for immutability.Use ReadonlyArray and precise label type to prevent accidental mutation and improve inference.
- enabledLegend?: Array<LegendItem>; - onSeriesToggle?: (label: string) => void; + enabledLegend?: ReadonlyArray<LegendItem>; + onSeriesToggle?: (label: LegendItem["label"]) => void;Also applies to: 33-35
84-88: Decouple enabledItems from color requirement (accept labels-only).Legend only reads labels from enabledItems; requiring color increases coupling. Consider narrowing to Pick<LegendItem, "label"> or a Set.
- enabledItems?: LegendItem[]; + enabledItems?: ReadonlyArray<Pick<LegendItem, "label">> | ReadonlySet<string>;If you choose Set, adjust enabledSet accordingly.
📜 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(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/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
🧠 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/features/insights/components/ChartCard.tsx
🧬 Code graph analysis (1)
packages/features/insights/components/ChartCard.tsx (2)
packages/features/insights/components/booking/EventTrendsChart.tsx (1)
legend(24-31)packages/features/insights/components/routing/RoutingFunnelContent.tsx (1)
legend(26-30)
🔇 Additional comments (5)
packages/features/insights/components/ChartCard.tsx (5)
39-42: Legend injection is backward-compatible.Conditional render keeps existing charts unaffected. LGTM.
89-91: Memoization and click gating look solid.Set-based lookup is O(1) and
Boolean(onItemToggle)is clear. LGTM.
101-106: Confirm opacity per spec (25% vs 50%).Design thread shows disagreement; spec earlier mentioned 50%. Please confirm and align.
- !isEnabled && "opacity-25" + !isEnabled && "opacity-50"
109-113: Verify tooltip on disabled buttons.Some browsers suppress pointer events on disabled controls; ensure Tooltip still triggers. If not, wrap button with a non-disabled parent that hosts Tooltip or move Tooltip outside button.
Example:
- <button ... disabled={!isClickable}> - ... - <Tooltip content={item.label}> - <span>...</span> - </Tooltip> - </button> + <Tooltip content={item.label}> + <button ... disabled={!isClickable}> + ... + <span>...</span> + </button> + </Tooltip>
98-100: Key uniqueness depends on label; confirm labels are unique across legend.If duplicates are possible, consider
${label}-${index}to avoid React key collisions.- <Fragment key={item.label}> + <Fragment key={`${item.label}-${index}`}>
E2E results are ready! |
3 participants
What does this PR do?
This PR adds toggleable legend functionality to ChartCard components, allowing users to click legend items to show/hide chart series with visual feedback. The implementation includes:
useToggleableLegendfor managing enabled/disabled legend stateLink to Devin run: https://app.devin.ai/sessions/3b59c972e51a4ceaa18083639a702794
Requested by: @eunjae-lee
Visual Demo (For contributors especially)
Screenshot.2025-08-27.at.11.38.29.mp4
Screenshot.2025-08-27.at.11.39.22.mp4
Expected behavior:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Environment setup: Standard Cal.com development environment with insights data
Key Areas for Review
Code Quality
Technical Implementation
Checklist