Skip to content

[Android][TextInput] Update types for autoComplete prop. - #25549

Closed
drtangible wants to merge 3 commits into
react:masterfrom
drtangible:update-types-for-autocomplete-prop
Closed

[Android][TextInput] Update types for autoComplete prop.#25549
drtangible wants to merge 3 commits into
react:masterfrom
drtangible:update-types-for-autocomplete-prop

Conversation

@drtangible

Copy link
Copy Markdown
Contributor

Summary

I believe there's a mismatch between the type definitions and the expected prop in Android for TextInput's autoComplete prop.

  • Android is expecting autoComplete.
  • JS types are expecting autoCompleteType.
  • Latest documentation documents autoCompleteType.

Prop added here: 179d490

This change updates the JS types to match what Android is expecting (autoComplete). Can update documentation if this is the approach we'd prefer (rather than updating Android to expect autoCompleteType).

Changelog

[Javascript] [Fixed] - Update types for TextInput's autoComplete prop.

Test Plan

Before:

  • Pass invalid value to TextInput's autoComplete prop, see no type errors on JS side, and Android blows up with:
Invalid autocomplete option: foobar
updateViewProp
ViewManagersPropertyCache.java:95
setProperty
ViewManagerPropertyUpdater.java:132
updateProps
ViewManagerPropertyUpdater.java:51
updateProperties
ViewManager.java:37

After:

  • Pass invalid value to TextInput's autoComplete prop, see PropType warning for autoComplete prop.

@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 Jul 8, 2019
@react-native-botreact-native-bot added Component: TextInput Related to the TextInput component. Platform: Android Android applications. Bug labels Jul 8, 2019
@drtangible
drtangibleforce-pushed the update-types-for-autocomplete-prop branch 3 times, most recently from 1485feb to b754ec0CompareJuly 8, 2019 20:46

@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.

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

@rickhanlonii

rickhanlonii commented Jul 12, 2019

Copy link
Copy Markdown
Contributor

Hey @drtangible, thanks for submitting your first RN PR!

This is a great find! This change would be a breaking change for the TextInput component API which we should avoid if possible. There are two ways to do that:

  • Update android to change the prop as you mentioned
  • Change the prop as it's passed to native

The first option is ideal, but the second is a quick fix. To do the second option you can change TextInput here to be something like:

consttextContainer=(<AndroidTextInput{...props}autoComplete={props.autoCompleteType}// .../>);

This will keep the prop we use in JS as autoCompleteType but send autoComplete to Android.

You can choose either route 👍

@drtangible

Copy link
Copy Markdown
ContributorAuthor

@rickhanlonii Sounds good! Pushed a change on the Android side, but can try to take a look this weekend about anything else that's required for native-side changes.

@mdvaccamdvacca 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.

Changes look good to me!
Thanks for working on this @drtangible

@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.

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

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @drtangible in daa6b0a.

When will my fix make it into a release? | Upcoming Releases

@rickhanlonii

Copy link
Copy Markdown
Contributor

@yungsters did you mean to re-open this?

@yungsters

Copy link
Copy Markdown
Contributor

Whoah, what. No.

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

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Component: TextInputRelated to the TextInput component.MergedThis PR has been merged.Platform: AndroidAndroid applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@drtangible@rickhanlonii@react-native-bot@yungsters@mdvacca@facebook-github-bot