Skip to content
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

Merged
merged 27 commits into from
Jan 22, 2025
Merged

New component - PieChart #3470

merged 27 commits into from
Jan 22, 2025

Conversation

nitzanyiz
Copy link
Contributor

@nitzanyiz nitzanyiz commented Dec 26, 2024

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

@nitzanyiz nitzanyiz changed the title Feat/new component pie chart New component - Pie Chart Dec 26, 2024
@nitzanyiz nitzanyiz changed the title New component - Pie Chart New component - PieChart Dec 26, 2024
@nitzanyiz nitzanyiz requested a review from ethanshar December 26, 2024 13:50
@nitzanyiz nitzanyiz assigned nitzanyiz and ethanshar and unassigned nitzanyiz Dec 26, 2024
@nitzanyiz nitzanyiz marked this pull request as ready for review December 26, 2024 13:51
@ethanshar
Copy link
Collaborator

@nitzanyiz
When creating a new component, always create an example screen with it
It also makes it easier for the reviewer to check the new component

@nitzanyiz
Copy link
Contributor Author

@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.

@nitzanyiz nitzanyiz assigned ethanshar and unassigned nitzanyiz Jan 6, 2025
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Wrote a few comment

demo/src/screens/MenuStructure.js Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/components/piechart/index.tsx Outdated Show resolved Hide resolved
src/components/piechart/PartialCircle.tsx Outdated Show resolved Hide resolved
src/components/piechart/PartialCircle.tsx Outdated Show resolved Hide resolved
src/components/piechart/index.tsx Outdated Show resolved Hide resolved
src/components/piechart/index.tsx Outdated Show resolved Hide resolved
src/components/piechart/PartialCircle.tsx Outdated Show resolved Hide resolved
@ethanshar ethanshar assigned nitzanyiz and unassigned ethanshar Jan 8, 2025
@nitzanyiz
Copy link
Contributor Author

@ethanshar , I made all the changes and added some to the api (border width and color). Also I added the api json.

@nitzanyiz nitzanyiz assigned ethanshar and unassigned nitzanyiz Jan 8, 2025
@ethanshar ethanshar assigned nitzanyiz and unassigned ethanshar Jan 15, 2025
@ethanshar
Copy link
Collaborator

@nitzanyiz looks good, see my replies - left few small things. Ping me on slack when it's ready and I'll approve

@nitzanyiz nitzanyiz assigned ethanshar and unassigned nitzanyiz Jan 15, 2025
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote one last comment, you can fix and merge. I approved

Comment on lines 25 to 27
if (!Svg || !Path) {
console.error(`RNUILib PieChart requires installing "@react-native-svg" dependency`);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's return here null after showing the error to avoid an actual exception that might crash the whole screen or tests

@nitzanyiz nitzanyiz enabled auto-merge (squash) January 22, 2025 14:51
@nitzanyiz nitzanyiz merged commit 1ffc372 into master Jan 22, 2025
1 check passed
@nitzanyiz nitzanyiz deleted the feat/new-component-PieChart branch January 22, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants