New component - PieChart #3470
New component - PieChart #3470
Conversation
|
@nitzanyiz |
|
@ethanshar I think I need to rethink the api. When I started to use the component I got into a few problems with the api. Lets discuss this. |
| padding?: number; | ||
| }; | ||
|
|
||
| const DEFAULT_DIVIDER_COLOR = '#FFFFFF'; |
There was a problem hiding this comment.
I suggest using our Colors.$backgroundDefault here so in dark mode it won't stay white but use the default background color
There was a problem hiding this comment.
Is it ok to set it in the global scope of the file or should it be at the component render? my meaning is using const DEFAULT_DIVIDER_COLOR = Colors.$backgroundDefault; inside the component and not out side of it.
There was a problem hiding this comment.
For Branded and Editor stage purposes it's better if you'll set it directly when destructing the prop here
dividerColor = DEFAULT_DIVIDER_COLOR
src/components/piechart/index.tsx
Outdated
|
|
||
| export type PieChartProps = { | ||
| segments: PieChartSegmentProps[]; | ||
| size?: number; |
There was a problem hiding this comment.
consider adding a defaultSegmentProps where the user can control default UI for all segments like the divider color and width
There was a problem hiding this comment.
Added for the border width and color as you suggested.
There was a problem hiding this comment.
Cool, don't forget to add them also to the api.json
There was a problem hiding this comment.
Oh yeah. I forgot 👌
There was a problem hiding this comment.
Since you are using an optional dependency it's possible the user won't have it installed, what we usually do is warn the user so they'll know to install it when using this component
moreover don't render the component if it's not installed, otherwise it will throw the user an error
You can see a reference for this behavior in our Card component that depends on blurView dep
There was a problem hiding this comment.
I suggest moving the validation to the main component so the user won't get an error per segment but only once
|
@ethanshar , I made all the changes and added some to the api (border width and color). Also I added the api json. |
|
@nitzanyiz looks good, see my replies - left few small things. Ping me on slack when it's ready and I'll approve |
Description
Added a new component PieChart. The component gets segments which is an array of percentages to that each segment of the pie should cover. Each segment is a percentage representing the percentage of the pie to cover and a color for the pie segment.
Changelog
⭐ PieChart - New Component ⭐
Additional info
MADS-3418