Skip to content

Scott/chart styles#400

Open
sagrimson wants to merge 9 commits into
datacommonsorg:mainfrom
madebypxlp:scott/chart-styles
Open

Scott/chart styles#400
sagrimson wants to merge 9 commits into
datacommonsorg:mainfrom
madebypxlp:scott/chart-styles

Conversation

@sagrimson

Copy link
Copy Markdown
Contributor

Overview

Adds styled bar chart variants (vertical and horizontal) and line chart improvements to chart cards, replaces hardcoded chart dimensions with responsive containers, and fixes several layout issues around card resizing, table scrolling, and wheel event propagation.

Changes Made

  • Add DataChartBarVertical and DataChartBarHorizontal components using Recharts BarChart with design token colors, tooltips, compact number formatting, and custom cursors/axis lines
  • Refactor DataChartLine to use ResponsiveContainer instead of fixed width/height props; add tooltip, custom cursor, active dot styling, and full-width axis line
  • Wire up chart style selector so bar-vertical, bar-horizontal, and line each render the correct chart component
  • Fix card auto-height not updating when Recharts renders chart SVGs asynchronously by adding a MutationObserver alongside the existing ResizeObserver
  • Fix wheel/scroll propagation in card base — walk from event target up to container to detect any scrollable ancestor, rather than only checking the container itself
  • Style the data table view: alternating row backgrounds, sticky thead, standard (non-compact) number formatting, and bottom margin
  • Make the chart/table tab panel scrollable when it contains a table (sticky header, overflow-y: auto)
  • Increase chart card max height from 520 → 720 to accommodate taller responsive charts
  • Adjust chart container CSS to flex-grow and remove bottom padding so charts fill available space

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces horizontal and vertical bar charts, refactors the line chart to be responsive, and updates card auto-height calculations to handle asynchronous SVG rendering via a MutationObserver. Feedback focuses on optimizing wheel event propagation to prevent layout thrashing, fixing visual misalignment bugs in custom chart cursors, simplifying the auto-height observer architecture to avoid performance issues, and removing hardcoded locales to support internationalization.

Comment thread dataweaver/apps/web/src/components/elements/card/base.tsx
Comment thread dataweaver/apps/web/src/components/elements/card/use_card_auto_height.ts Outdated
thead {
position: sticky;
top: 0;
z-index: 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use:

Suggested change
z-index: 1;
z-index: $z-index-above-content;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

<m.div
key={activeTab.label}
role="tabpanel"
className={clsx(s.panel)}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
className={clsx(s.panel)}
className={s.panel}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! done

Comment on lines +16 to +18
const BAR_COLOR = `rgb(${COLORS['card-surface-selected']})`;
const GRID_COLOR = `rgb(${COLORS['card-chart-grid']})`;
const AXIS_COLOR = `rgb(${COLORS['card-content-muted']})`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reckon all chart colours should probably just be directly tokenized for ease. I.e:

Suggested change
const BAR_COLOR = `rgb(${COLORS['card-surface-selected']})`;
const GRID_COLOR = `rgb(${COLORS['card-chart-grid']})`;
const AXIS_COLOR = `rgb(${COLORS['card-content-muted']})`;
const BAR_COLOR = `rgb(${COLORS['card-chart-bar']})`;
const GRID_COLOR = `rgb(${COLORS['card-chart-grid']})`;
const AXIS_COLOR = `rgb(${COLORS['card-chart-axis']})`;

@sagrimson sagrimson force-pushed the scott/chart-styles branch from 9dbc8ff to f7150cd Compare July 7, 2026 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants