Uh oh!
There was an error while loading. Please reload this page.
ReactActivity extends FragmentActivity - #22662
Conversation
dulmandakh
commented
Dec 17, 2018
LinusU
commented
Dec 17, 2018
This looks great! 👏 |
facebook-github-bot
left a comment
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.
dulmandakh
commented
Dec 20, 2018
@hramos anything I can help with? |
hramos
commented
Dec 20, 2018
@dulmandakh the internal diff is passing all tests. I'm waiting for someone to take another look internally before I land it. |
dulmandakh
commented
Dec 21, 2018
@hramos cool. Once merged, I'll do some cleanup in other parts. |
hramos
left a comment
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?
Uh oh!
There was an error while loading. Please reload this page.
dulmandakh
commented
Jan 12, 2019
@hramos done |
dulmandakh
commented
Jan 14, 2019
leaving only public ReactActivityDelegate(ReactActivity activity, @nullable String mainComponentName), because it'll work for both ReactActivity and ReactFragmentActivity, which extends ReactActivity. |
facebook-github-bot
left a comment
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
left a comment
There was a problem hiding this comment.
It looks like the ReactActivityDelegate that takes an Activity argument is still removed. Ideally, it would still be there, but marked as deprecated.
dulmandakh
commented
Jan 16, 2019
@hramos ReactActivityDelegate now have 2 constructors for both Activity and ReactActivity, and constructor for Activity is marked as deprecated. |
facebook-github-bot
left a comment
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.
dulmandakh
commented
Feb 1, 2019
@hramos any news? |
hramos
commented
Feb 1, 2019
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. |
react-native-bot
commented
Feb 1, 2019
@dulmandakh merged commit dda2b82 into |
Summary: In react/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: react/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.