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

use AppCompat widgets #19768

Closed
wants to merge 12 commits into from
Closed

Conversation

dulmandakh
Copy link
Contributor

Android documentation recommends to use widgets from AppCompat, also Android Studio warns about it. This PR changes Picker, Slider, Text, TextInput components to use AppCompat widgets.

CI is GREEN https://circleci.com/gh/dulmandakh/react-native/453

Test Plan

Android builds and runs as usual, almost no visual changes

Release Notes

[ANDROID] [ENHANCEMENT] [WIDGET] - extend widgets from AppCompat

@dulmandakh dulmandakh changed the title use Appcompat widgets use AppCompat widgets Jun 17, 2018
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 17, 2018
@react-native-bot react-native-bot added ✅Test Plan Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jun 17, 2018
@dulmandakh
Copy link
Contributor Author

These are widgets used when widgets (without appcompat) defined in XML layouts. Also this will enhance material design support, example 1723b6c.

For Slider, we'll be able to add support for image thumb. Feature parity with iOS.

@hramos

@dulmandakh
Copy link
Contributor Author

@hramos Please merge, I'm preparing a pull request that will add thumb image to Slider component (feature parity with iOS)

@dulmandakh
Copy link
Contributor Author

@mdvacca Please merge

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.

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

@dulmandakh
Copy link
Contributor Author

@hramos any news? Please cherry pick it to stable

@hramos
Copy link
Contributor

hramos commented Jun 25, 2018

@dulmandakh it's still waiting on a review from @mdvacca.

@mdvacca
Copy link
Contributor

mdvacca commented Jun 25, 2018

The code looks good to me, @dulmandakh can you describe what are the visual changes that you mentioned in the description?

@mdvacca
Copy link
Contributor

mdvacca commented Jun 25, 2018

@dulmandakh
Copy link
Contributor Author

@mdvacca I see no visual difference, but AppCompatSeekBar, AppCompatSpinner, AppCompatEditText, AppCompatTextView are introduced in support library 25.1.0 and used by default in layouts for compatibility.

excerpt from AppCompatSeekBar doc

This will automatically be used when you use SeekBar in your layouts and the top-level activity / dialog is provided by appcompat. You should only need to manually use this class when writing custom views.

excerpt from AppCompatSpinner doc

  • Allows dynamic tint of its background via the background tint methods in CompoundButtonCompat.
  • Allows setting of the background tint using buttonTint and buttonTintMode.
  • Setting the popup theme using popupTheme.

This will automatically be used when you use Spinner in your layouts. You should only need to manually use this class when writing custom views.

excerpt from AppCompatEditText doc

  • Allows dynamic tint of its background via the background tint methods in ViewCompat.
  • Allows setting of the background tint using backgroundTint and backgroundTintMode.

This will automatically be used when you use EditText in your layouts and the top-level activity / dialog is provided by appcompat. You should only need to manually use this class when writing custom views.

excerpt from AppCompatTextView doc

  • Allows dynamic tint of its background via the background tint methods in ViewCompat.
  • Allows setting of the background tint using backgroundTint and backgroundTintMode.
  • Supports auto-sizing via TextViewCompat by allowing to instruct a TextView to let the size of the text expand or contract automatically to fill its layout based on the TextView's characteristics and boundaries. The style attributes associated with auto-sizing are autoSizeTextType, autoSizeMinTextSize, autoSizeMaxTextSize, autoSizeStepGranularity and autoSizePresetSizes, all of which work back to Ice Cream Sandwich.

This will automatically be used when you use TextView in your layouts and the top-level activity / dialog is provided by appcompat. You should only need to manually use this class when writing custom views.

@dulmandakh
Copy link
Contributor Author

@mdvacca please approve 😄

@dulmandakh
Copy link
Contributor Author

@mdvacca please merge, it'll render widgets according to Google recommendations and allow us to support backgroundTint.

@facebook-github-bot
Copy link
Contributor

@dulmandakh I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

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.

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

@hramos
Copy link
Contributor

hramos commented Sep 12, 2018

Here's an example of a failure that just surfaced when this PR was imported:

Summary: 
 ** Summary of failures encountered during the build **
Rule //redacted// FAILED because Command failed with exit code 1.
stderr: react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java:24: error: cannot access android.support.v4.view.TintableBackgroundView
public class ReactTextView extends AppCompatTextView implements ReactCompoundView {
       ^
  class file for android.support.v4.view.TintableBackgroundView not found
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:43: error: First argument must be a subclass of View
  public void setNumberOfLines(ReactTextView view, int numberOfLines) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:48: error: First argument must be a subclass of View
  public void setEllipsizeMode(ReactTextView view, @Nullable String ellipsizeMode) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:63: error: First argument must be a subclass of View
  public void setTextAlignVertical(ReactTextView view, @Nullable String textAlignVertical) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:79: error: First argument must be a subclass of View
  public void setSelectable(ReactTextView view, boolean isSelectable) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:84: error: First argument must be a subclass of View
  public void setSelectionColor(ReactTextView view, @Nullable Integer color) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:103: error: First argument must be a subclass of View
  public void setBorderRadius(ReactTextView view, int index, float borderRadius) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:116: error: First argument must be a subclass of View
  public void setBorderStyle(ReactTextView view, @Nullable String borderStyle) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:130: error: First argument must be a subclass of View
  public void setBorderWidth(ReactTextView view, int index, float width) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:147: error: First argument must be a subclass of View
  public void setBorderColor(ReactTextView view, int index, Integer color) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:155: error: First argument must be a subclass of View
  public void setIncludeFontPadding(ReactTextView view, boolean includepad) {
              ^
react-native-github/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java:160: error: First argument must be a subclass of View
  public void setDisabled(ReactTextView view, boolean disabled) {
              ^

@dulmandakh
Copy link
Contributor Author

you need to add support-v4 as dependency

react_native_dep("third-party/android/support/v4:lib-support-v4")

and maybe support-annotations

react_native_dep("third-party/android/support-annotations:android-support-annotations")

@hramos
Copy link
Contributor

hramos commented Sep 13, 2018

Can you make the change on your PR? I'll re-import afterwards.

@dulmandakh
Copy link
Contributor Author

@hramos Android CI is green 👍

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 19, 2018
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.

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Sep 20, 2018
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@dulmandakh
Copy link
Contributor Author

@hramos please try again 👍

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.

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

@hramos
Copy link
Contributor

hramos commented Jan 29, 2019

Should we continue working on this?

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Jan 30, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants