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

Migrate usages of @react-native-community/art to react-native-svg #229

Merged
merged 8 commits into from
Jul 10, 2021
Merged

Conversation

frw
Copy link
Contributor

@frw frw commented Jun 27, 2021

@react-native-community/art could be deprecated soon: react-native-art/art#75
This pull requests replaces usages of @react-native-community/art with the well-supported react-native-svg library.

This should also resolve #183

@frw frw mentioned this pull request Jun 27, 2021
@frw
Copy link
Contributor Author

frw commented Jun 27, 2021

Tested with Example app:

screen-20210627-135912_Trim.mp4

@oblador oblador self-requested a review June 30, 2021 14:26
Copy link
Owner

@oblador oblador left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for this, I know many have been looking forward to this PR. I've updated the Example app, fixed some minor lockfile misses so that it works with latest version of Xcode etc.

Overall it looks good to me, one minor regression however is that the progress circle in your PR is different from master behavior. Should be an easy fix, see the expected animation in the video below:

master.example.mp4

@frw
Copy link
Contributor Author

frw commented Jun 30, 2021

@oblador Found the issue. It seems that passing in a rotation transform to the Svg component in Circle.js when it's no longer rotating causes it to be flipped. I'm not exactly sure why this is the case, but found a workaround to pass in an undefined style when there is no rotation and this seems to have fixed it.

screen-20210701-010820.mp4

@frw frw requested a review from oblador June 30, 2021 18:31
@asklar
Copy link

asklar commented Jul 6, 2021

CC @marlenecota - any concerns about functionality in svg that this PR depends on, re: the Windows support you're adding, or should we be good?

@@ -8,13 +8,13 @@ Progress indicators and spinners for React Native using ReactART.

`$ npm install react-native-progress --save`

### ReactART based components
### React Native SVG based components

Choose a reason for hiding this comment

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

There is still a reference to ReactART above ☝️ on line 3 https://github.com/oblador/react-native-progress/pull/229/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R3

More importantly: thank you for proposing this PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - Fixed it!

Copy link

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Looks like it's in an "all comments addressed, needs hopefully-final review" state, so I approve, completely without any authority to do so 😆

@frw
Copy link
Contributor Author

frw commented Jul 9, 2021

@oblador Could you take a quick look at this?

Copy link
Owner

@oblador oblador left a comment

Choose a reason for hiding this comment

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

Champ!

@oblador oblador merged commit afd3d70 into oblador:master Jul 10, 2021
@oblador
Copy link
Owner

oblador commented Jul 10, 2021

Released in 5.0.0

@mikehardy
Copy link

Likewise @oblador thanks for your work here and on react-native-vector-icons, I use both in my projects to great effect, I really appreciate it. Cheres

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.

Doesn't work in Expo SDK36
4 participants