-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add Fabric support #90
Add Fabric support #90
Conversation
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.
LGTM! 🎉
view.setStyle(styleName); | ||
} | ||
|
||
@ReactProp(name = ViewProps.COLOR, customType = "Color") | ||
@Override | ||
@ReactProp(name = ViewProps.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.
Why is customType = "Color"
gone 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.
Because of adding processColor
in JS the prop stopped working when customType = "Color"
was there.
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.
Hmm looking e.g. here: https://github.com/software-mansion/react-native-svg/blob/1126079425a1b1a6c6094434f1c199c52ded1817/src/fabric/AndroidSvgViewNativeComponent.ts#L33 and then here: https://github.com/software-mansion/react-native-svg/blob/18a2f93e4057bae3dbdb604d9590861450f23b11/android/src/main/java/com/horcrux/svg/SvgViewManager.java#L101 and here: https://github.com/software-mansion/react-native-svg/blob/18a2f93e4057bae3dbdb604d9590861450f23b11/android/src/main/java/com/horcrux/svg/SvgViewManager.java#L101 and here: https://github.com/software-mansion/react-native-svg/blob/1126079425a1b1a6c6094434f1c199c52ded1817/src/elements/Svg.tsx#L179, maybe we should not use processColor
in JS but just do something similar to how it is done in svg?
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.
Why was processColor
added in the first place?
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.
Done in 45c5d5e. I don't particularly remember why I've done it this way 😅.
namespace react | ||
{ | ||
|
||
Size RNCProgressBarMeasurementsManager::measure( |
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.
Could you write a comment from where did you take analogous code with some links and a short explanation of what is done here if needed?
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.
Added in 722b62f.
Hi there ! @WoLewicki @j-piasecki Do you need a hand to merge it? I really need it so I could help if you want... |
We don't have the rights to merge it unfortunately 😞 You can try and ping the library maintainers to do it. |
@Naturalclar this PR does not provide support for RN 0.71, but we will soon make a PR with it. Would be nice to merge and release it then 🚀 |
Here it is: #96 |
PR adding Fabric support to the library. Custom cpp state implementation is inspired by
react-native-screens
and the measurement methods are mainly taken from RN core.Closes #87