-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add percentFormatter prop to PieWidgetUI #844
Conversation
Pull Request Test Coverage Report for Build 8157955159Details
💛 - Coveralls |
Visit the preview URL for this PR (updated for commit 0fb4914): https://cartodb-fb-storybook-react-dev--pr844-feat-add-percent-r2b0bjhh.web.app (expires Tue, 12 Mar 2024 14:36:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c |
@@ -281,6 +294,7 @@ PieWidgetUI.propTypes = { | |||
colors: PropTypes.array, | |||
formatter: PropTypes.func, | |||
tooltipFormatter: PropTypes.func, | |||
percentFormatter: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export the new prop from types packages/react-ui/src/types.d.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -49,6 +49,7 @@ function PieWidgetUI({ | |||
data = [], | |||
formatter, | |||
tooltipFormatter, | |||
percentFormatter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work also in ComparativePieWidget
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't implement it in ComparativePieWidget, I could implement this feature in comparative mode, but maybe could be far away of the scope of this feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's ok for you, no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ComparativePie Widget was indeed coming from PS, if I'm not wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's wait for @VictorVelarde review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - PS
Description
Add to central text in PieChartUI the same text formatter
Acceptance