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

a proposed fix to the strokeCap issue number #44 #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SiyabongaNzulwana
Copy link

@SiyabongaNzulwana SiyabongaNzulwana commented Aug 27, 2020

As I have mentioned that I was facing an issue with the strokeCap={'round'} prop, I was able to figure out what the issue might be, and I decided to push a PR for the proposed fix. NB: this is happening on "version": "1.1.2".

This is related to issue number #44

Screenshot 2020-08-24 at 16 58 08

@SiyabongaNzulwana SiyabongaNzulwana changed the title a proposed fix to the strokeCap issue number 11 a proposed fix to the strokeCap issue number #44 Aug 27, 2020
@zgordon01
Copy link
Collaborator

I think this will break this use case:
round

After applying your changes I see this:

roundbroken

You also have merge conflicts as I just merged #40 which ran prettier on the Pie.js file.

src/Pie.js Outdated
@@ -156,7 +156,7 @@ const CleanUpCircles = ({dimensions, backgroundColor, visible}) => {
}

const Pie = ({ sections, radius, innerRadius, backgroundColor, strokeCap, dividerSize }) => {
strokeCapForLargeBands = dividerSize > 0 || strokeCap == 'butt' ? 'butt' : 'butt';
Copy link
Author

Choose a reason for hiding this comment

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

@nihgwu I see what you mean, however this should definitely be a typo, I will pull and push a PR to fix this then, and revert the if((width) > 100 && visible){ to if((width) < 100 && visible){ but this is not fixing the issue number #44.

were you guys able to reproduce this issue on your side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, that ternary doesn't make much sense as it is right now, likely a typo

@zgordon01
Copy link
Collaborator

@SiyabongaNzulwana ready for review again?

@SiyabongaNzulwana
Copy link
Author

@zgordon01 Yes, its ready...

@CarolynWebster
Copy link

This same line is currently causing our app to crash because the is no const before the variable name. Would you consider updating this, and then @nihgwu can we get this merged in?

My guess is this repo is abandoned at this point, but hopeful!

@@ -224,7 +224,7 @@ const Pie = ({
dividerSize,
}) => {
strokeCapForLargeBands =

Choose a reason for hiding this comment

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

Can we add const before establishing the variable?

@nihgwu
Copy link
Owner

nihgwu commented Apr 26, 2022

@CarolynWebster Sorry I don't use RN for years, but if you can confirm this PR works, I'll update your propose directly and merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants