-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Android][TextInput] Update types for autoComplete prop. #25549
Conversation
1485feb
to
b754ec0
Compare
There was a problem hiding this 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.
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:
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 You can choose either route 👍 |
@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. |
There was a problem hiding this 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
There was a problem hiding this 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.
This pull request was successfully merged by @drtangible in daa6b0a. When will my fix make it into a release? | Upcoming Releases |
@yungsters did you mean to re-open this? |
Whoah, what. No. |
Summary
I believe there's a mismatch between the type definitions and the expected prop in Android for
TextInput
'sautoComplete
prop.autoComplete
.autoCompleteType
.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 expectautoCompleteType
).Changelog
[Javascript] [Fixed] - Update types for
TextInput
'sautoComplete
prop.Test Plan
Before:
TextInput
'sautoComplete
prop, see no type errors on JS side, and Android blows up with:After:
TextInput
'sautoComplete
prop, see PropType warning forautoComplete
prop.