fix(pie): apply percentage number format to labels - #42880
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42880 +/- ##
==========================================
+ Coverage 57.10% 66.37% +9.26%
==========================================
Files 2857 2857
Lines 161125 161049 -76
Branches 37060 37049 -11
==========================================
+ Hits 92014 106890 +14876
+ Misses 68259 52143 -16116
- Partials 852 2016 +1164
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #472e84Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
@EvanGruhlkey thanks for the PR! Since there is a visual component to your change, can you add before and after screenshots of your fix into the PR description? |
| const percentFormatter = numberFormat.endsWith('%') | ||
| ? getNumberFormatter(numberFormat) | ||
| : defaultPercentFormatter; |
There was a problem hiding this comment.
Suggestion: Calling endsWith directly on numberFormat can throw when an older or partially populated chart payload explicitly provides number_format: undefined (or null). The merged form data then overwrites DEFAULT_PIE_FORM_DATA, while getValueFormatter already accepts an absent format. Guard the value before checking its suffix so such charts continue to render. [null pointer]
Severity Level: Major ⚠️
- ❌ Legacy pie charts with null formats fail transformation.
- ❌ Pie labels and tooltips do not render.
- ⚠️ Dashboard hydration preserves malformed form values.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
**Line:** 303:305
**Comment:**
*Null Pointer: Calling `endsWith` directly on `numberFormat` can throw when an older or partially populated chart payload explicitly provides `number_format: undefined` (or `null`). The merged form data then overwrites `DEFAULT_PIE_FORM_DATA`, while `getValueFormatter` already accepts an absent format. Guard the value before checking its suffix so such charts continue to render.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
The flagged issue is correct. Calling Here is the corrected implementation for const percentFormatter = numberFormat?.endsWith('%')
? getNumberFormatter(numberFormat)
: defaultPercentFormatter;There are no other comments on this pull request to address. superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts |
Apply a selected percentage-style D3 number format to pie chart percentages instead of always using the hardcoded two-decimal formatter. Non-percentage number formats retain the existing percentage formatting behavior.
This affects percentage values rendered in pie labels, templates, tooltips, and grouped "Other" tooltip rows.
Fixes #42834