CartoDB / carto-react

CARTO for React packages

https://docs.carto.com/react/

MIT License

38 stars 16 forks source link

Add formatter to central text in PieWidgetUI #843

Closed jantolg closed 8 months ago

jantolg commented 9 months ago

Description

Add to central text in PieChartUI the same text formatter

Slack thread
Feature

Acceptance

Please describe how to validate the feature or fix

Go to PieWidgetUI
Use a custom formatter
Check the central text as the same format as you set at formatter func

github-actions[bot] commented 9 months ago

Pull Request Test Coverage Report for Build 8099436624

Details

8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 68.596%

Totals
Change from base Build 7956818395:	0.04%
Covered Lines:	2630
Relevant Lines:	3491

💛 - Coveralls

github-actions[bot] commented 9 months ago

Visit the preview URL for this PR (updated for commit abd2547):

https://cartodb-fb-storybook-react-dev--pr843-feat-add-central-nmfupptg.web.app

_{(expires Thu, 07 Mar 2024 16:42:13 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c}

VictorVelarde commented 9 months ago

The approach LGTM, just 2 caveats:

The new option to add a formatter deserves a story in storybook, don't you think so? That way it will show it in real action.
Default behaviour: I think we should preserve current behaviour, so keep old formatter if non-formatter is passed down. Otherwise users will get something like https://cartodb-fb-storybook-react-dev--pr843-feat-add-central-nmfupptg.web.app/?path=/docs/widgets-piewidgetui--default

Thx for the help here @jantolg

jantolg commented 9 months ago

I've just to add an example of storybook, a new changelog entry, and a default behaviour of formatter based on useIntl.formatNumber

jantolg commented 8 months ago

I close this PR.

Upon reviewing the behavior, maybe using the same formatter for units and percentage values is not a good approach, as it represents two different types of values. After discussing this issue with @aaranadev and @menusal, we believe it would be better to have a separate formatter for each type of value.

I have created a new PR with these changes to avoid mixing different approaches.

NOTE: It might be worth considering a standardized approach for using formatters, perhaps by unifying all formatters into a single property or something similar. This would be a good topic to open a discussion about for future C4R versions

Continue in this new PR #844