Skip to content

Add new players chart#461

Open
tudddorrr wants to merge 1 commit intodevelopfrom
new-players-chart
Open

Add new players chart#461
tudddorrr wants to merge 1 commit intodevelopfrom
new-players-chart

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Feb 16, 2026
@claude
Copy link

claude bot commented Feb 16, 2026

PR Review: Add new players chart

Overall this is clean work - the component decomposition is solid, the SWR pattern is consistent with the codebase, and the ChartTooltipEventChartTooltip rename is handled correctly everywhere.


🔵 Minor Issues

1. No loading indicator in BarChartCard

When loading=true and data is empty (initial fetch), the component renders nothing - no spinner, no skeleton. The empty message is correctly hidden, but there is no visual feedback to the user.

// BarChartCard.tsx
{!loading && data.length === 0 &&
  <p>{emptyMessage}</p>
}

// data.length > 0 check renders chart, but nothing renders during initial load

Consider adding a loading state (skeleton or spinner) so the card area does not appear blank during the first fetch.


2. Positive change values lack a + prefix

In NewPlayersChart.tsx, the percentage display shows -5.0% for negative change but 5.0% (no sign) for positive, making it visually asymmetric:

{(change * 100).toFixed(1)}%

A small fix makes it unambiguous:

{change > 0 ? `+${(change * 100).toFixed(1)}` : (change * 100).toFixed(1)}%

3. No tests for new components

BarChartCard, BarChartTooltip, and NewPlayersChart have no test files. The renamed EventChartTooltip has tests, so there is a precedent. At minimum, BarChartTooltip (which has conditional rendering and a formatter callback) would benefit from unit tests consistent with the existing chart test pattern.

@tudddorrr tudddorrr force-pushed the new-players-chart branch 2 times, most recently from 49ab594 to 24f65a5 Compare February 17, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant