[BUGFIX] PieChart: Decimal, Unit & Short Values attributes not taken …#594
[BUGFIX] PieChart: Decimal, Unit & Short Values attributes not taken …#594adrianSepiol wants to merge 3 commits intoperses:mainfrom
Conversation
…into account Signed-off-by: Adrian Sepiół <adisepko@gmail.com>
adrianSepiol
force-pushed
the
fix-piechart-plugin-decimal-unit-and-short
branch
from
881f679 to
3fc393a
Compare
Gladorme
left a comment
Gladorme
left a comment
There was a problem hiding this comment.
You have it also in screenshots, there is double %% in tooltip 🤔
piechart/src/PieChartPanel.tsx
Outdated
| const { legendItems, legendColumns } = useMemo(() => { | ||
| const pieChartLegendMapper: PieChartLegendMapper = | ||
| pieChartLegend?.mode === 'table' ? new PieChartTableLegendMapper() : new PieChartListLegendMapper(); | ||
| const values = pieChartLegend?.values as ComparisonValues[] | undefined; |
There was a problem hiding this comment.
If possible:
| const values = pieChartLegend?.values as ComparisonValues[] | undefined; | |
| const values: ComparisonValues[] | undefined = pieChartLegend?.values; |
There was a problem hiding this comment.
This wouldn't work as values is Array<LegendValue | ComparisonValues>. I could make downstream function accept this type and just check for the values that interest us ('abs', 'relative'). This way we would avoid casting.
There was a problem hiding this comment.
I prefer to avoid casting when possible, would be nice to downstream the type and just check for the values that interest us, like you said
Signed-off-by: Adrian Sepiół <adisepko@gmail.com>
Good catch, I think I looked at it for too long and stopped seeing it |
| const relativeFormatOptions = { unit: 'percent', decimalPlaces: formatOptions?.decimalPlaces } as const; | ||
| return ({ name, value, percent }: formatterProps): string => { | ||
| if (typeof value === 'number') { | ||
| return `${format.encodeHTML(name)}: ${formatValue(value, formatOptions)} (${formatValue(percent, relativeFormatOptions)})`; |
There was a problem hiding this comment.
We never used encodeHTML, I quickly checked the doc, maybe we should use it everywhere.
Or we don't put it anywhere and we suppose datasource should be safe 🤔
There was a problem hiding this comment.
Nothing to change just a remark for the future ^^
Signed-off-by: Adrian Sepiół <adisepko@gmail.com>
2 participants
…into account
closes: PieChart: Decimal, Unit & Short Values attributes not taken into account
Description
Fixed issue where decimal, unit and short values where not applied. After the change formatting is applied to absolute and relative values in legends table, on chart labels and tooltip. Additionally fixed issue where tooltip showed relative value in "percentage" mode
Screenshots
Before
After
Before in percentage mode:
After in percentage mode:
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes