Skip to content

use AppCompat widgets - #19768

Closed
dulmandakh wants to merge 12 commits into
react:masterfrom
dulmandakh:appcompat-widget
Closed

use AppCompat widgets#19768
dulmandakh wants to merge 12 commits into
react:masterfrom
dulmandakh:appcompat-widget

Conversation

@dulmandakh

Copy link
Copy Markdown
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

@dulmandakhdulmandakh changed the title use Appcompat widgetsuse AppCompat widgetsJun 17, 2018
@facebook-github-botfacebook-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-botreact-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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@mdvacca Please merge

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

@hramos any news? Please cherry pick it to stable

@hramos

Copy link
Copy Markdown
Contributor

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

@mdvacca

Copy link
Copy Markdown
Contributor

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

@mdvacca

Copy link
Copy Markdown
Contributor

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
ContributorAuthor

@mdvacca please approve 😄

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

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

@facebook-github-bot

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@hramos Android CI is green 👍

@facebook-github-botfacebook-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

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

Copy link
Copy Markdown
Contributor

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-botfacebook-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

@hramos please try again 👍

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Should we continue working on this?

@dulmandakh

dulmandakh commented Jan 30, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: AndroidAndroid applications.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@dulmandakh@hramos@mdvacca@facebook-github-bot@react-native-bot