feat: Support string value in Pie outerRadius callbacks#6191
feat: Support string value in Pie outerRadius callbacks#6191ckifer merged 3 commits intorecharts:mainfrom
outerRadius callbacks#6191Conversation
|
Updates to recharts.org documentation coming soon |
|
One question: do we have a pattern for allowing users to create custom functions in storybook? I was wondering how to show the user-defined functions as a parameter in the storybook example |
Bundle ReportChanges will increase total bundle size by 118 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6191 +/- ##
=======================================
Coverage 96.74% 96.74%
=======================================
Files 222 222
Lines 20019 20019
Branches 4140 4140
=======================================
Hits 19368 19368
Misses 645 645
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
https://storybook.js.org/docs/writing-stories/args#mapping-to-complex-arg-values
|
|
Thanks for the PR! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for string values as return types from outerRadius callbacks in Pie charts, extending the existing functionality that only supported number returns from callbacks.
- Updated type definitions to allow callbacks to return both numbers and strings
- Modified the
getOuterRadiusfunction to handle string returns from callbacks usinggetPercentValue() - Added comprehensive unit tests for both string and number callback returns
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/polar/Pie.tsx |
Updated getOuterRadius function to use getPercentValue() for callback return values and modified type definitions |
src/state/types/PieSettings.ts |
Updated type definition to allow callbacks to return both numbers and strings |
test/polar/Pie.spec.tsx |
Added unit tests for string and number callback returns in outerRadius |
storybook/stories/API/polar/Pie.stories.tsx |
Updated documentation to clarify callback can return strings or numbers |
| { name: 'C', value: 20 }, | ||
| ]; | ||
|
|
||
| const outerRadiusCallback = (dataPoint: any) => { |
There was a problem hiding this comment.
Consider using a more specific type instead of any for the dataPoint parameter. Based on the test data structure, it should be { name: string; value: number }.
| const outerRadiusCallback = (dataPoint: any) => { | |
| const outerRadiusCallback = (dataPoint: { name: string; value: number }) => { |
| { name: 'C', value: 20 }, | ||
| ]; | ||
|
|
||
| const outerRadiusCallback = (dataPoint: any) => { |
There was a problem hiding this comment.
Consider using a more specific type instead of any for the dataPoint parameter. Based on the test data structure, it should be { name: string; value: number }.
| const outerRadiusCallback = (dataPoint: any) => { | |
| const outerRadiusCallback = (dataPoint: { name: string; value: number }) => { |
| const sectorPaths = Array.from(sectors).map(sector => sector.getAttribute('d')); | ||
| expect(sectorPaths[0]).not.toBe(sectorPaths[1]); | ||
| expect(sectorPaths[1]).not.toBe(sectorPaths[2]); | ||
| expect(sectorPaths[0]).not.toBe(sectorPaths[2]); |
There was a problem hiding this comment.
[nitpick] The test logic for verifying different sector sizes could be more robust. Consider checking actual computed radius values or path dimensions instead of just comparing path strings, as path differences don't guarantee the intended radius behavior.
| expect(sectorPaths[0]).not.toBe(sectorPaths[2]); | |
| // Helper to extract the outer arc endpoint from SVG path and compute radius | |
| function getOuterRadiusFromPath(d: string | null, cx: number, cy: number): number | null { | |
| if (!d) return null; | |
| // Find the first arc command (A) and its endpoint | |
| // Example: M x0 y0 A rx ry xAxisRotation largeArcFlag sweepFlag x1 y1 ... | |
| const arcMatch = d.match(/A\s*[\d.]+\s*[\d.]+\s*[\d.]+\s*[01]\s*[01]\s*([\d.]+)\s*([\d.]+)/); | |
| if (!arcMatch) return null; | |
| const x = parseFloat(arcMatch[1]); | |
| const y = parseFloat(arcMatch[2]); | |
| // Compute distance from center | |
| return Math.sqrt(Math.pow(x - cx, 2) + Math.pow(y - cy, 2)); | |
| } | |
| const cx = 250, cy = 250; | |
| const expectedRadii = [0.6 * 250, 0.8 * 250, 1.0 * 250]; // 60%, 80%, 100% of 250 | |
| const sectorPaths = Array.from(sectors).map(sector => sector.getAttribute('d')); | |
| const computedRadii = sectorPaths.map(d => getOuterRadiusFromPath(d, cx, cy)); | |
| // Check that computed radii are close to expected | |
| computedRadii.forEach((radius, i) => { | |
| expect(radius).not.toBeNull(); | |
| expect(radius!).toBeCloseTo(expectedRadii[i], 1); | |
| }); |
| const sectorPaths = Array.from(sectors).map(sector => sector.getAttribute('d')); | ||
| expect(sectorPaths[0]).not.toBe(sectorPaths[1]); | ||
| expect(sectorPaths[1]).not.toBe(sectorPaths[2]); | ||
| expect(sectorPaths[0]).not.toBe(sectorPaths[2]); |
There was a problem hiding this comment.
[nitpick] The test logic for verifying different sector sizes could be more robust. Consider checking actual computed radius values or path dimensions instead of just comparing path strings, as path differences don't guarantee the intended radius behavior.
| expect(sectorPaths[0]).not.toBe(sectorPaths[2]); | |
| function extractRadiusFromPath(d: string | null, cx: number, cy: number): number | null { | |
| if (!d) return null; | |
| const match = d.match(/M\s*([-\d.]+)\s*,?\s*([-\d.]+)/); | |
| if (!match) return null; | |
| const x = parseFloat(match[1]); | |
| const y = parseFloat(match[2]); | |
| return Math.sqrt(Math.pow(x - cx, 2) + Math.pow(y - cy, 2)); | |
| } | |
| const expectedRadii = [100, 150, 200]; | |
| const actualRadii = Array.from(sectors).map((sector, i) => | |
| extractRadiusFromPath(sector.getAttribute('d'), 250, 250) | |
| ); | |
| actualRadii.forEach((radius, i) => { | |
| expect(radius).not.toBeNull(); | |
| expect(radius!).toBeCloseTo(expectedRadii[i], 1); | |
| }); |
Description
Adds support for string values as returned by
outerRadiuscallback in Pie.Related Issue
5998
Motivation and Context
Currently, Pie supports number and string values, which are handled by the
getPercentValue()util. Additionally, Pie supports callbacks that return number values, which are not handled and instead returned directlyThis PR adds support for callbacks that return string values, by using
getPercentValue()to handle the returned string/number from the callback.How Has This Been Tested?
Added unit test to Pie's test suite, testing the execution of
outerRadiuscallbacks that return numbers and strings.I ran the tests locally, using
npm run test.Screenshots (if appropriate):
N/A
Types of changes
Checklist: