Conversation
|
This actually looks good, I'll share it with some Meta devs to get more feedback, but well done. 🥳 |
WoLewicki
left a comment
WoLewicki
left a comment
There was a problem hiding this comment.
I added some comments. It looks good overall 🎉
android/build.gradle
Outdated
| @@ -133,8 +143,8 @@ dependencies { | |||
|
|
|||
| if (isNewArchitectureEnabled()) { | |||
There was a problem hiding this comment.
Is it if still necessary if you target the newest RN version with your library?
There was a problem hiding this comment.
My goal was to treat the new architecture as opt-in. Maybe removing this check here does not cause any trouble, but I'd keep it for safety.
There was a problem hiding this comment.
Sorry, what I meant is that since RN 0.70, all those lines are not needed: https://github.com/Kureev/react-native-blur/pull/493/files/1af1251593436cff9015115886fdfd0edd025268#diff-197b190e4a3512994d2cebed8aff5479ff88e136b8cc7a4b148ec9c3945bd65aR144-R149.
There was a problem hiding this comment.
Lines removed; I moved javaPackageName to Codegen config in package.json.
ios/BlurViewComponentView.mm
Outdated
| @@ -0,0 +1,72 @@ | |||
| #ifdef RCT_NEW_ARCH_ENABLED | |||
There was a problem hiding this comment.
Do you need to separate this to another component? Maybe it would be easier for BlurView to extend RCTViewComponentView on new arch and UIView on old arch and make it implement the needed methods on new arch? Or am I missing something?
There was a problem hiding this comment.
Deleted the extra BlurViewComponentView and used #ifdef guards in the original BlurView.
ios/VibrancyViewComponentView.h
Outdated
| @@ -0,0 +1,15 @@ | |||
| #ifdef RCT_NEW_ARCH_ENABLED | |||
There was a problem hiding this comment.
Same here as for BlurView, since VibrancyView would still inherit from BlurView, which would inherit from RCTViewComponentView, it would work correctly then. It's done this way e.g. in react-native-svg for all components: https://github.com/react-native-svg/react-native-svg/blob/f49728d783c952a97c29a8bdb3b4ad688fca2330/apple/RNSVGNode.h and e.g.: https://github.com/react-native-svg/react-native-svg/blob/f49728d783c952a97c29a8bdb3b4ad688fca2330/apple/Elements/RNSVGDefs.mm
There was a problem hiding this comment.
Deleted the extra VibrancyViewComponentView; now it's directly inheriting from BlurView. I also removed the updateProps method, as it's the same as the one BlurView implements.
|
Thanks for the review @WoLewicki. I refactored the code based on your comments. |
|
Hi! First of all, thank you @gispada for this work! I'm wondering when this branch is going to be merged :) I checked it in my project, everything works fine. |
* build(deps): update react-native * feat: codegen setup for BlurView component * feat: add basic Fabric component for BlurView (iOS) * feat(iOS): implement updateProps Fabric method * feat(iOS): migrate VibrancyView * feat(Android): add code for new and old architecture * chore: update dependencies and example app * fix(iOS): interface VibrancyViewComponentView * refactor: separate codegen specs by platform * refactor: rename Android component file to avoid a bug in Codegen * refactor: delete/rename files * refactor: conditionally include Fabric code in the original views * refactor: remove unnecessary code in build.gradle
* added mavenCentral() for gradle builds (margelo#418) * refactor: typescript, hooks, build.gradle & podspec fixes and moved t… * chore: update Readme.md to reflect new maintainer * feat: update android blurview to 2.0.2 (margelo#478) * feat: extend android properties (margelo#481) * refactor: migrate example app to use RNTA (margelo#484) * fix(android): build issue on compileSdkVersion < 31 (margelo#485) * chore: release 4.2.0 * feat: Add Fabric support (margelo#493) * chore: release 4.3.0 * chore: update README.md (margelo#494) * New "transparent" blurType that turns every blurEffectView subview ba… * Add "transparent" blur type to README.md, BlurView.ios.tsx and BlurVi… * Fix blurType in VibrancyViewNativeComponent.ts * feat(iOS): transparent blur type * Merge branch 'master' into pr/513
4 participants
Closes #487
The goal of this PR is to add Fabric support, while keeping the library compatible with the old architecture.
I've also updated several packages, including of course React and React Native. The example app has been updated as well, even though the segmented control used there does not yet support Fabric.
The majority of the work is done, I am testing my changes.I think it's ready to be merged now, status changed to Ready for review.Feedback is welcome, especially from the library maintainers, and also from everybody that is willing to test this migration.