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: support Fabric on Android #1804

Merged
merged 82 commits into from
Aug 12, 2022
Merged

feat: support Fabric on Android #1804

merged 82 commits into from
Aug 12, 2022

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Jul 20, 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.

@WoLewicki WoLewicki marked this pull request as ready for review July 22, 2022 09:14
@WoLewicki WoLewicki mentioned this pull request Jul 22, 2022
@WoLewicki WoLewicki changed the title @wolewicki/add fabric android feat: support Fabric on Android Jul 22, 2022
Comment on lines +39 to +43
public void setCx(String cx) {
mCx = SVGLength.from(cx);
invalidate();
}

Copy link
Member

Choose a reason for hiding this comment

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

I think I remember you explaining problems with Dynamic and CodeGen. If that's the reason behind every setter being duplicated for String and Dynamic it would be great to have a comment explaining it somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I will add a comment in the main PR's description.

Copy link

@mateosilguero mateosilguero left a comment

Choose a reason for hiding this comment

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

It tooks 15min but i works well for me 👍🏻

@andyoh365
Copy link

This is a great milestone! Are you also working on the iOS platform by any chance?

@WoLewicki
Copy link
Member Author

@andyoh365 iOS part is available in this PR (see the target branch of this PR) and the changes are visible in #1754.

@andyoh365
Copy link

Thank you!

@andyoh365
Copy link

Hi, when I yarn add this branch or the latest commit from this branch or the #add-fabric branch, I get the error message below. When I check react-native-svg folder under node_modules, the lib folder is missing, which exists in the latest stable version.

Error: While trying to resolve module react-native-svg from file /Users/______/Documents/GitHub/rn-new-arch-test/src/REA3Test3.js, the package /Users/______/Documents/GitHub/rn-new-arch-test/node_modules/react-native-svg/package.json was successfully found. However, this package itself specifies a main module field that could not be resolved (/Users/______/Documents/GitHub/rn-new-arch-test/node_modules/react-native-svg/lib/commonjs/index.js. Indeed, none of these files exist:

  • /Users/______/Documents/GitHub/rn-new-arch-test/node_modules/react-native-svg/lib/commonjs/index.js(.native|.ios.js|.native.js|.js|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
  • /Users/______/Documents/GitHub/rn-new-arch-test/node_modules/react-native-svg/lib/commonjs/index.js/index(.native|.ios.js|.native.js|.js|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)

@WoLewicki
Copy link
Member Author

Yeah, lib folder is not added to github repo, but is added to npm repository on release, therefore you won't get lib folder when downloading repo from a commit. The easiest solution would be to run yarn inside the repo's folder and it should generate the lib folder then. Could you check if it works?

@WoLewicki WoLewicki changed the base branch from @wolewicki/add-fabric to main August 11, 2022 12:08
@WoLewicki WoLewicki marked this pull request as draft August 12, 2022 07:34
@WoLewicki WoLewicki marked this pull request as ready for review August 12, 2022 08:00
@WoLewicki WoLewicki merged commit 77267be into main Aug 12, 2022
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.

5 participants