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

fix arc drawing when sweep >= 360 #15042

Closed
wants to merge 7 commits into from
Closed

Conversation

nihgwu
Copy link
Contributor

@nihgwu nihgwu commented Jul 17, 2017

I hit this issue in my own component https://github.com/nihgwu/react-native-pie, and find this https://stackoverflow.com/questions/19383842/weird-behaviour-in-drawing-a-ring-using-path-arcto-in-android, after this change, it looks exactly the same on both Android and iOS

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 17, 2017
@nihgwu
Copy link
Contributor Author

nihgwu commented Jul 17, 2017

Sample code:

      <View style={styles.container}>
        <Pie
          radius={100}
          series={[10, 20, 30, -40]}
          colors={['red', 'lime', 'blue', 'yellow']} />
        <Pie
          radius={100}
          innerRadius={60}
          series={[10, 20, 30, 400]}
          colors={['#f00', '#0f0', '#00f', '#ff0']} />
        <View>
          <Pie
            radius={50}
            innerRadius={45}
            series={[100]}
            colors={['#f00']}
            backgroundColor='#ddd' />
          <View style={styles.gauge}>
            <Text style={styles.gaugeText}>100%</Text>
          </View>
        </View>
      </View>

After:
image

Before:
image

On iOS:
image

nihgwu added a commit to nihgwu/react-native-pie that referenced this pull request Jul 17, 2017
@nihgwu
Copy link
Contributor Author

nihgwu commented Jul 18, 2017

cc @javache @shergin

@pull-bot
Copy link

pull-bot commented Aug 7, 2017

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

@shergin
Copy link
Contributor

shergin commented Aug 7, 2017

@nihgwu That's great!
One question: How the demo will look (on iOS) with 50% transparent colors? Could you please create a snack with your demo?

@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 8, 2017

@shergin https://snack.expo.io/By3_faIPb
I changed the last value from 400 to 140 slightly, but the whole ring is still covered by the last color, seems SVG doesn't support transparent color?

@nihgwu
Copy link
Contributor Author

nihgwu commented Sep 8, 2017

@shergin Any thing should I do to make this be merged?

@shergin
Copy link
Contributor

shergin commented Sep 8, 2017

Android documentation said:

sweepAngle: Sweep angle (in degrees) measured clockwise, treated mod 360.

So, it means that it already does not support arcs "bigger" that 360 deg (but iOS does, I assume), so it means that this should not introduce a regression here.

Let's try to land it on Monday.

@nihgwu
Copy link
Contributor Author

nihgwu commented Sep 28, 2017

@shergin another release is coming, can we make this in?

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Sep 28, 2017
@facebook-github-bot
Copy link
Contributor

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants