ReactActivity extends FragmentActivity #22662
Conversation
facebook-github-bot
added
the
CLA Signed
|
This looks great! 👏 |
There was a problem hiding this comment.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@hramos anything I can help with? |
|
@dulmandakh the internal diff is passing all tests. I'm waiting for someone to take another look internally before I land it. |
|
@hramos cool. Once merged, I'll do some cleanup in other parts. |
There was a problem hiding this comment.
Thanks for the pull request. @mdvacca reviewed it internally and he agrees it's a good idea to use FragmentActivity instead of plain Activity.
We're requesting two changes to your pull request:
-
Please consider marking
ReactActivityDelegateas deprecated before removing it. Once the deprecation notice makes it to a release, we can then removeReactActivityDelegatefor the next release. -
Can you edit the original PR description's changelog to make note of the deprecation?
ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java
Show resolved
Hide resolved
|
@hramos done |
|
leaving only public ReactActivityDelegate(ReactActivity activity, @nullable String mainComponentName), because it'll work for both ReactActivity and ReactFragmentActivity, which extends ReactActivity. |
There was a problem hiding this comment.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@hramos ReactActivityDelegate now have 2 constructors for both Activity and ReactActivity, and constructor for Activity is marked as deprecated. |
There was a problem hiding this comment.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@hramos any news? |
|
Thanks for the reminder! It looks like the internal diff got approved. I just kicked off the land process, if everything goes well, you should see a message from the bot posted here later today. |
|
@dulmandakh merged commit dda2b82 into |
Summary: In facebook#20602, I tried to make ReactActivity to extend AppCompatActivity per Google recommendation. But import failed, now ReactActivity extends FragmentActivity which is a parent class of AppCompatActivity and step forward to extend AppCompatActivity. Pull Request resolved: facebook#22662 Reviewed By: mdvacca Differential Revision: D13505140 Pulled By: hramos fbshipit-source-id: d4edc8dc5c606c45811c1deddf5727a47ad484d8
Summary: In facebook/react-native#20602, I tried to make ReactActivity to extend AppCompatActivity per Google recommendation. But import failed, now ReactActivity extends FragmentActivity which is a parent class of AppCompatActivity and step forward to extend AppCompatActivity. Pull Request resolved: facebook/react-native#22662 Reviewed By: mdvacca Differential Revision: D13505140 Pulled By: hramos fbshipit-source-id: d4edc8dc5c606c45811c1deddf5727a47ad484d8
In #20602, I tried to make ReactActivity to extend AppCompatActivity per Google recommendation. But import failed, now ReactActivity extends FragmentActivity which is a parent class of AppCompatActivity and step forward to extend AppCompatActivity.
Test Plan:
Everything should run as usual.
Changelog:
[Android] [Changed] - ReactActivity extends FragmentActivity. Therefore, we're deprecating ReactFragmentActivity and it'll will be removed in next release.