Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New component - PieChart #3470
base: master
Are you sure you want to change the base?
New component - PieChart #3470
Changes from 12 commits
020ee68
7e67b6d
946865b
f255a3c
4654d42
cfc1a85
b7c9e80
c908a35
6a2d2da
d17152b
622300a
ede9897
07e9217
6cf616b
1817bf5
3aeeafd
b6d84cb
645e547
ec6f9a1
0775c74
56c30c5
5849aa3
94af279
4bf82d2
62d23fa
9724813
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving the validation to the main component so the user won't get an error per segment but only once
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Branded and Editor stage purposes it's better if you'll set it directly when destructing the prop here
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.
consider adding a
defaultSegmentProps
where the user can control default UI for all segments like the divider color and widthThere 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.
Added for the border width and color as you suggested.
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.
Cool, don't forget to add them also to the api.json
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.
Oh yeah. I forgot 👌