Repository navigation
Conversation
Fix typo and remove TODO
|
|
||
| public class ReactFragment extends Fragment implements PermissionAwareActivity, DefaultHardwareBackBtnHandler { | ||
|
|
||
| public final static int REQUEST_OVERLAY_CODE = 1111; |
There was a problem hiding this comment.
Why is this needed? Can you add a comment please?
There was a problem hiding this comment.
Yeah, its basically a duplication of the [ReactActivityDelegate] (https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java#L34) which relies on the permission system to allow dev/debug mode.
Edit: I can add comments, or as mentioned in your summary comment I can attempt to share the code between this and the delegate class?
| private static final String REDBOX_PERMISSION_MESSAGE = | ||
| "Overlay permissions need to be granted in order for react native apps to run in dev mode."; | ||
| private static final String REDBOX_PERMISSION_GRANTED_MESSAGE = | ||
| "Overlay permissions have been granted."; |
There was a problem hiding this comment.
Why are these needed? Can you add a comment?
There was a problem hiding this comment.
See comment above ^^
| handled = true; | ||
| } | ||
| } | ||
| return handled; |
There was a problem hiding this comment.
This looks like copy & paste? Can you extract it to a shared helper rather than copy & pasting?
|
The need for using RN in fragments is understandable. The idea seems good to me. However, how much of the code here is copied from somewhere else in the codebase? It's going to be very hard to maintain the exact same copy of the logic in two different places - it's guaranteed people will only update one place and forget to update the other. If we want to commit this, every non-trivial piece of logic that's been copied from somewhere will need to be extracted to a shared helper (e.g. a class with helpers that's used by both the activity and fragment). Looks like the code doesn't compile? |
|
@mkonicek great insight and thanks for reviewing the code!
|
…ctFragment * upstream/master: (23 commits) packager: fix uncaught rejection `[-RCTUIManager setFrame:forView:]` was replaced with `[-RCTUIManager setSize:forView:]` Allow specific border width styling Added property display: flex and none Add border support to ScrollViews refactor Module.js Random @providesModule related fixes Android: Add onScroll event to TextInput Support the --remote option in bump-oss-version.js script packager: reestablish changes reverted by d63f9c Forcing git diff to not use colorized output Added missing files to React.xcodeproj removed bestander from email notifications Add perspective transform support on Android Better ListView - FlatList Rename files to match @providesModule annotations Fixed crash in RCTScrollView on iOS 8 Fix <SwipeableRow /> layout. Eject CLI command to re-create native folders Fix NetworkingModule losing Cookies when multiple CatalystInstances exist and one is destroyed ...
I'm currently not to sold on the re-use of this commit, but wanted to take it a step forward and see if anything good could come out of it. I welcome strict critticism and hope to improve this code. I still think a Fragment would be valuable, but sharing the necessary methods between the ReactActivity and making sure to point out methods that can't be shared is required.
|
@mkonicek I've added a commit to fix this build failures, as well as attempt to re-use the existing code from the I foresee something like the following: The benefit of the Activity is less native code and easier to get a "green-field" project started. |
serj-lotutovici
left a comment
There was a problem hiding this comment.
This is something I was defiantly missing. Hoping to see this in a stable version.
One side note. The consuming activity needs to override at least two methods, which probably should be documented somewhere.
|
|
||
| public void onHostResume() { | ||
| if (getReactNativeHost().hasInstance()) { | ||
| if (mActivity instanceof DefaultHardwareBackBtnHandler) { |
There was a problem hiding this comment.
This forces the host activity to implement the interface, when in fact it's already implemented by the ReactFragment (that btw is never used).
There was a problem hiding this comment.
Good call out. How would you see this working then? Extra param in the constructor to pass a DefaultHardwareBackBtnHandler object which can either be a fragment or an activity that implements the interface?
There was a problem hiding this comment.
That would be my first choice. I wonder if the the behaviour would be the same, since ReactActivity is calling super.onBackPressed() and the fragment only calls getActivity().onBackPressed(). The chain of thought is that the host activity will have to call the fragment's onBackPressed method do decide whether or not to call the super method.
There was a problem hiding this comment.
I'm actually implementing react-navigation this week in a project that will make use of the back button login, in this exact fragment class, so I can tweak it a bit to figure out what feels best. But I think I agree with what you're saying above.
There was a problem hiding this comment.
@serj-lotutovici So I updated the code a bit in b089982
After messing with fragment navigations stacks this week, it seemed easier to allow the Activity to handle the back navigation. If the Fragment handled the back button handler, it ended up getting stuck in a loop, or it wasn't able to safely call the super.onBackPressed() like I was hoping for. I'm open to suggestions but this is the conclusion I came to.
This will be shown via documentation for sure, but I'll work on documentation if/when we get this PR "approved" and the idea is solidified.
There was a problem hiding this comment.
@jpshelley I agree on the importance of Fragment support. For example some components like ViewPager return Fragments for each view. This is standard. Btw, are there tests for the Fragment solution or do you have an example app that is using these modifications?
There was a problem hiding this comment.
We are currently in the process of open sourcing our solution at Hudl. Since contributing back to React Native seems to be taking longer then expected, we wanted to be able to share our approach with others. I will comment back on this PR when that happens.
There was a problem hiding this comment.
Sounds great! I am currently doing React Native component integration into Native app and would like to try it out when available.
…ctFragment * upstream/master: (148 commits) Match on `s.version` only API update and bug fixes Clean up separator stuff Improve flow typing Remove unused RCTMessageQueue Make flow types update to date with proptypes with ImageSource Move some utilities to RCTCxxUtils Fail safely if there are extra DrawView commands in ClippingDrawCommandManager Deleting Optimized Bundle! Support QuickPerformanceLogger.js in the old bridge Revert D4559304: FBReactKit/js: upgrade babel dependencies Revert D4559167: packager: introducing async/await Removed duplicate code by calculating with mainSize/crossSize Fix up some small issues with YGUnitAuto Move JSBigString to its own header Move RCTFollyConvert to the correct namespace Change cursor color when using selectionColor on Android BREAKING: Change the js1 - packager - RN App protocol to version 2 Allow listener for native Animated.event Update grammar and typos in documentation ...
* master: add Systrace to all PerformanceLogger.js markers Guard against missing native module Fix CI runs Move tryBytecode to bundle specifier Support QuickPerformanceLogger.js in CxxBridge Add missing reference image for tvOS ImageSnapshotTest Move initializedExecutorWithContextProvider API
|
I've ironed out more of the The change I mention is similar to the idea I showed above via the diagram. The delegate will handle all common operations between a Let me know what people think about:
|
|
Any thoughts on the build errors? Looks like a lot Flow related issue of |
|
@jpshelley Is there any example code you can push to github or bitbucket using the ReactFragment? This would help in it's use and review. |
|
@jpshelley Any chance you can show a example code for using with a view pager? I tried the solution at http://stackoverflow.com/questions/35221447/react-native-inside-a-fragment, but the fragment don`t detect touches to it, so I can't perform any actions with it. Thank you |
|
For anyone following this PR, we are looking at open sourcing our solution at Hudl. From there we can add examples on how to include with ViewPagers or other Fragment based solutions. |
|
@jpshelley can you briefly list the instructions to get this working without waiting for a merge into the RN codebase (so moving all three classes into a separate namespace and adding it). I'm trying to get RN implemented in my current company and need to integrate it with an existing android app that uses fragments |
| if (mPermissionListener != null && mPermissionListener.onRequestPermissionsResult(requestCode, permissions, grantResults)) { | ||
| mPermissionListener = null; | ||
| } | ||
| mPermissionsCallback = args -> { |
There was a problem hiding this comment.
Could you change this to not use a lambda expression? We are still supporting older versions of Java that don't have them.
There was a problem hiding this comment.
Yup definitely. Curious question, what version of Java do you back support and is there a specific reason why? Just wanting to learn from it.
|
@jpshelley that's great! @mdvacca will do a review soon and we can hopefully get this landed finally. In the meantime, can you get the android tests to pass on CI? One of the issues is that you are using lambda expressions which unfortunately we cannot use yet. |
| @@ -15,8 +15,6 @@ | |||
|
|
|||
| import com.facebook.infer.annotation.Assertions; | |||
| import com.facebook.react.bridge.Callback; | |||
There was a problem hiding this comment.
Before looking to merge this - does it make sense to have a ReactActivityDelegate and then a ReactDelegate or does it make sense for ReactActivity to use the ReactDelegate directly? Was trying to bring smaller changes with this PR and focus primarily on the addition of the ReactFragment but also don't want to incur possible Tech Debt
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mdvacca is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was successfully merged by @jpshelley in 691f2f9. When will my fix make it into a release? | Upcoming Releases |
|
OMG thank you @mdvacca for merging this PR. It's been open for over two years and I'm glad we could finally bring this functionality into React Native, especially since a ton of people have asked for it. @jpshelley I also want to thank you for sticking with us for all this time and going through numerous reviews and rebases. I cannot imagine myself how demoralizing it must have been to wait for this long but I am incredibly thankful that you did. Since you sent this PR, the React Native team at Facebook changed completely and we try to spend more time on open source to bring your contributions into the repo faster. I hope to see many more PRs from you in the future, and I promise we'll get to them much faster. For now, would you be willing to add documentation for this functionality to the React Native website? Finally, thanks @mkonicek for reviewing this PR a long time ago and helping so much on open source at FB! I know how much work it was and that it's easy to drown in it. |
|
We reverted this commit because it caused a bug internally at Facebook. I will work on a fix and bring the code back in a commit soon. |
|
Anything to help make the environment and framework better @cpojer 🤝 @mdvacca - let me know if you need any help at all. and finally, is there any plans to update documentation/examples/blog posts etc? Would be happy to help on that front as well. As soon as Hudl makes use of the merged code in a future release, we'll write up our own blog post to sunset https://github.com/hudl/react-native-android-fragment as well, with a migration plan in place. |
|
@mdvacca any progress on the fix? Would love to help if needed. |
|
@patrickkempff this requires some changes internally at FB, I didn't have the time to take a look into it yet but I'm planning to work on it the next few weeks I will let you know If I need an extra hand! |
|
Today we fixed and landed this PR again. |
|
@mdvacca great news, awesome! |
|
Any tutorial/guide on how to use this functionality? I'd like to basically follow what is listed here https://facebook.github.io/react-native/docs/integration-with-existing-apps for Android but replace the activity with fragments as my application is based entirely around fragments. I cannot tell if this is already included in a release version of React Native. I am using 0.60.4, but the ReactFragment class cannot be found EDIT: This feature seems to not be present in 0.60.4 is there any timeline for when then this will be included in a release @mdvacca |
|
@saltyskip until its released you can also leverage this library - https://github.com/hudl/react-native-android-fragment this was the original work that got merged into master here. The API/Implementation should remain the same, so you can check out the example app. Once Hudl is able to use the React-Native version, I'll follow-up and add documentation to React Native and an example, if someone else doesn't beat me to it. |
|
Thanks @jpshelley got it to work with your original library. Great work, and looking forward to when this functionality is included in an official release :) |
|
@mdvacca public class MainActivity extends ReactActivity {
@Override
protected String getMainComponentName() {
return "Example";
}
+ @Override
+ protected ReactActivityDelegate createReactActivityDelegate() {
+ return new ReactActivityDelegate(this, getMainComponentName()) {
+ @Override
+ protected ReactRootView createRootView() {
+ return new RNGestureHandlerEnabledRootView(MainActivity.this);
+ }
+ };
+ }
}Currently(RN 0.61), there isnt a interface to do that |
|
I found my way to this PR after trying to add react-navigation to an old brownfield app that's slowly been migrated over the years up to RN 0.59. It seems to use the strategy in https://facebook.github.io/react-native/docs/integration-with-existing-apps, but those docs say nothing about |
|
If you want to use React, then ReactActivity is the way to go. |
|
That's great to hear, thanks. Is there any documentation about it, since apparently the official docs are out-of-date? |
How to solve this issue? Currently, only way is to rewrite |
|
Hello, Unfortunately, that's not possible private static ReactFragment newInstance(String componentName, Bundle launchOptions)This should be public static Bundle getReactFragmentBundle(String componentName, Bundle launchOptions)This way I can get a bundle that's required by your ReactFragment, and add my own args in it. Any plans on fixing this? |
|
For any future folks with @mitsest's question. You can use the |

Overview
React Native on Android has currently been focused and targeted at using an Activity for its main form of instantiation.
While this has probably worked for most companies and developers, you lose some of the modularity of a more cohesive application when working in a "brown-field" project that is currently native. This hurts more companies that are looking to adopt React Native and slowly implement it in a fully native application.
A lot of developers follow Android's guidelines of using Fragments in their projects, even if it is a debated subject in the Android community, and this addition will allow others to embrace React Native more freely. (I even assume it could help with managing navigation state in applications that contain a decent amount of Native code and would be appreciated in those projects. Such as sharing the Toolbar, TabBar, ViewPager, etc in Native Android)
Even with this addition, a developer will still need to host the fragment in an activity, but now that activity can contain native logic like a Drawer, Tabs, ViewPager, etc.
Test plan (required)
/Examplesfolder and make use of the Fragment, or even create a new example that uses a native layout manager like Drawer, Tabs, Viewpager, etc)Make sure tests pass on both Travis and Circle CI.
To Note:
Release Notes
[ANDROID][FEATURE][ReactAndroid/src/main/java/com/facebook/react/ReactFragment.java] - Adds support for Android's Fragment system. This allows for a more hybrid application.