-
Notifications
You must be signed in to change notification settings - Fork 348
refactor: Add ChartContainer component with toolbar #1560
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.
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
Conversation
🦋 Changeset detectedLatest commit: 378adc9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review - PR #1560✅ No critical issues found. The refactoring introduces a clean ChartContainer abstraction that consolidates chart toolbar logic across multiple components. The implementation follows existing patterns and maintains type safety. Observations (non-blocking):
Minor notes:
Overall, this is a solid refactoring that improves code organization and maintainability. ✅ |
E2E Test Results✅ All tests passed • 56 passed • 4 skipped • 719s
Tests ran across 4 shards in parallel. |
f9d9c14 to
e1dff08
Compare
My concern there is that the edit/delete icons only appear on hover, which would then push the display switcher to the left when the user goes to click it We could make the edit/delete buttons persistent (ie. show up even without hovering) but my thought was that that might be too many icons then, so I kept the current behavior. |
Good point - I agree! Disregard :) |
| children | ||
| ) : ( | ||
| <div | ||
| // Hack, recharts will release real fix soon https://github.com/recharts/recharts/issues/172 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point- we should bump recharts version :P
# Summary This PR adds the hover edit/duplicate/delete buttons back to the search tile. They were inadvertently removed as part of #1560 . ## Before <img width="813" height="430" alt="Screenshot 2026-01-09 at 11 48 32 AM" src="https://github.com/user-attachments/assets/393f402b-b64a-4633-8261-dd169ba2c011" /> ## After <img width="814" height="421" alt="Screenshot 2026-01-09 at 11 48 10 AM" src="https://github.com/user-attachments/assets/9afc368a-5b12-462e-8909-61fe6a581615" />


Closes HDX-2748
Summary
This PR introduces a new ChartContainer component which is used to wrap various Chart components, and provides a standardized way to put titles and toolbar icons above charts.
Motivation: we have various items above charts already (display switchers, titles, materialized view optimization icons) and we plan to add additional ones (for noting when time ranges have been updated to align with chart or MV granularities). With this change, we can put all of the items in the same row, which does not overlap with the chart data (Closes HDX-2748) and consolidate some logic.
Demo
Preset Dashboards
Custom Dashboards
Chart Explorer
Infra Side Panel