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

ReactActivity extends AppCompatActivity #23278

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Feb 3, 2019

Summary

Google recommends to extend AppCompatActivity. and This PR changes ReactActivity to extend AppCompatActivity

Changelog

[Android] [Changed] - ReactActivity extends AppCompatActivity

Test Plan

CI is green and everything should work as usual.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Core Team labels Feb 3, 2019
@pull-bot
Copy link

pull-bot commented Feb 3, 2019

Warnings
⚠️

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Feb 3, 2019

@mdvacca I just realized that I had learned something about BUCK setup while working on landing Android Support Library 28.0.0. Please import it and see if it works. Thank you very much.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Feb 4, 2019

@mdvacca is it work?

@mdvacca
Copy link
Contributor

mdvacca commented Feb 4, 2019

There is an internal build error because of lack of dependency but buck works fine, I will update that dependency and then land this

@react-native-bot
Copy link
Collaborator

@dulmandakh merged commit 3b9604f into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 5, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 5, 2019
@mdvacca
Copy link
Contributor

mdvacca commented Feb 6, 2019

This PR produced runtime crashes with errors:
You need to use a Theme.AppCompat theme (or descendant) with this activity.

The fix is to use a Theme from AppCompat in the AndroidManifest.xml file:
e.g.:
`
Replace the theme from:
android:theme="@android:style/Theme.NoTitleBar"

to:
android:theme="@style/Theme.AppCompat.Light.NoActionBar"
`

We should flag this PR as a backward in-compatible change for next version release.
CC @cpojer, @hramos

@dulmandakh
Copy link
Contributor Author

@mdvacca I couldn't find non AppCompat theme reference in RN repo. But I agree that we should add notes about it.

matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
Summary:
Google recommends to extend AppCompatActivity. and This PR changes ReactActivity to extend AppCompatActivity

[Android] [Changed] - ReactActivity extends AppCompatActivity
Pull Request resolved: facebook#23278

Reviewed By: fkgozali

Differential Revision: D13937917

Pulled By: mdvacca

fbshipit-source-id: 4216482b30349b24e3cbf2bdc24d9a890744132f
@dulmandakh dulmandakh deleted the reactactivity-extends-appcompatactivity branch February 22, 2019 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants