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

feat: add Fabric on iOS without ComponentViews #1821

Merged
merged 93 commits into from
Aug 11, 2022

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Aug 2, 2022

Summary

Version of #1754 without usage of ComponentViews. It seems like a more proper way, but introduces the necessity of clearing whole state of each component on recycling for it not to be used when view is recycled.

Still known problems:

  • We stringify props of type NumberProp since codegen only accepts props of a single type. It is the fastest way of dealing with it, but maybe there could be a better way to handle it.
  • Image resolving should be probably handled the same as in RN core
  • SvgView needs to set opaque = NO so it is does not have black background (it comes from the fact that RCTViewComponentView overrides backgroundColor setter which in turn somehow messes with the view being opaque). All other svg components do it already so maybe it is not such an issue.
  • transform prop won't work when set natively since it is not parsed in Fabric

@WoLewicki WoLewicki mentioned this pull request Aug 2, 2022
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I left some comments regarding React Native 0.69+, but then I noticed that you went for 0.68.1 in FabricExample, so thats fine.

Pointed some style-regarding things below.

.github/workflows/ios-build-test-fabric.yml Outdated Show resolved Hide resolved
source 'https://rubygems.org'

# You may use http://rbenv.org/ or https://rvm.io/ to install and use this version
ruby '2.7.4'
Copy link
Member

Choose a reason for hiding this comment

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

Ruby was bumped up to '2.7.5' with React Native 0.69.0 however I doubt it would cause any problems.

Comment on lines +31 to +32
libfolly_futures \
libfolly_json \
Copy link
Member

Choose a reason for hiding this comment

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

This also was changed in recent React Native versions.

Suggested change
libfolly_futures \
libfolly_json \
libfolly_runtime \

src/fabric/SvgViewNativeComponent.ts Outdated Show resolved Hide resolved
apple/Elements/RNSVGImage.mm Outdated Show resolved Hide resolved
@WoLewicki WoLewicki marked this pull request as ready for review August 10, 2022 08:12
@WoLewicki WoLewicki merged commit 8f1bda4 into main Aug 11, 2022
WoLewicki added a commit that referenced this pull request Aug 12, 2022
Most of Android changes for Fabric and bump of FabricExample to RN 0.69.2. iOS and JS changes are available in #1821.
The most notable change on Android is adding methods to components that accept String values of each NumberProp instead of Dynamic. Another change is changed structure of RenderableViewManager.java since we needed to abstract methods that belong only to components inheriting from VirtualView in order to be able to properly override them in their children.
aaron1234567890123 added a commit to aaron1234567890123/react-native-svg that referenced this pull request Jun 8, 2024
Version of software-mansion#1754 without usage of ComponentViews. It seems like a more
proper way, but introduces the necessity of clearing whole state of
each component on recycling for it not to be used when view is
recycled.

Still known problems:

We stringify props of type NumberProp since codegen only accepts props
of a single type. It is the fastest way of dealing with it, but maybe
there could be a better way to handle it.
Image resolving should be probably handled the same as in RN core
SvgView needs to set opaque = NO so it is does not have black
background (it comes from the fact that RCTViewComponentView overrides
backgroundColor setter which in turn somehow messes with the view
being opaque). All other svg components do it already so maybe it is
not such an issue.
transform prop won't work when set natively since it is not parsed in
Fabric
aaron1234567890123 added a commit to aaron1234567890123/react-native-svg that referenced this pull request Jun 8, 2024
Version of software-mansion#1754 without usage of ComponentViews. It seems like a more
proper way, but introduces the necessity of clearing whole state of
each component on recycling for it not to be used when view is
recycled.

Still known problems:

We stringify props of type NumberProp since codegen only accepts props
of a single type. It is the fastest way of dealing with it, but maybe
there could be a better way to handle it.
Image resolving should be probably handled the same as in RN core
SvgView needs to set opaque = NO so it is does not have black
background (it comes from the fact that RCTViewComponentView overrides
backgroundColor setter which in turn somehow messes with the view
being opaque). All other svg components do it already so maybe it is
not such an issue.
transform prop won't work when set natively since it is not parsed in
Fabric
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.

2 participants