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

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

Conversation

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

@rickhanlonii
Copy link
Member

rickhanlonii commented Jul 12, 2019

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:

const textContainer = (
  <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
Contributor Author

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

Copy link
Contributor

@mdvacca mdvacca left a comment

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

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 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
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
Member

@yungsters did you mean to re-open this?

@yungsters
Copy link
Contributor

Whoah, what. No.

@yungsters yungsters closed this Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants