-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
feat: Add id prop to Text, TouchableWithoutFeedback and View components #34522
Conversation
Base commit: e8739e9 |
Base commit: 73abcba |
@necolas has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@gabrieldonadel Wow you're on fire with all these PRs! 👏 |
Hi @lunaleaps, thanks for the kind words. I've just pushed a PR updating the docs (facebook/react-native-website#3285) and another one for DefinitelyTyped (DefinitelyTyped/DefinitelyTyped#62019) |
4609f1b
to
f4a9901
Compare
@necolas has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -49,6 +49,7 @@ type Props = $ReadOnly<{| | |||
disabled?: ?boolean, | |||
focusable?: ?boolean, | |||
hitSlop?: ?EdgeInsetsProp, | |||
id?: ?string, |
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.
Hi @gabrieldonadel, thanks for this PR. However, I don't think that if we are already using an optional property (id?
) it should also have an optional type ?string
.
Also, this definition conflicts with some other internal definition we have where the prop is defined as
id?: string
Could you update the PR so I can try to reimport it and merge it?
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.
Sure, I've just updated it
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.
The internal failure was because a component was defining id
for custom purposes and didn't strip it from the props it forwarded to the RN components. We should check what the type is in React DOM, and patch the internal RN code if this type should be an optional string...although I don't understand why Flow would complain about receiving a string for an optional string value.
06045fd
to
863a4a2
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
863a4a2
to
9fa3cf8
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
d818f94
to
76a7beb
Compare
76a7beb
to
a3ecb80
Compare
This pull request was successfully merged by @gabrieldonadel in 3e97d5f. When will my fix make it into a release? | Upcoming Releases |
…ts (facebook#34522) Summary: This adds the `id` prop to `Text`, `TouchableWithoutFeedback` and `View` components as requested on facebook#34424 mapping the existing `nativeID` prop to `id`. As this components are inherited by others this also adds the `id` prop support to `Image`, `TouchableBounce`, `TouchableHighlight`, `TouchableOpacity` and `TextInput`. This PR also adds android tests ensuring that the `id` property is passed to the native view via the `nativeID` prop, these tests were based on the existing `nativeID` tests ([NativeIdTestCase.java](https://github.com/facebook/react-native/blob/main/ReactAndroid/src/androidTest/java/com/facebook/react/tests/NativeIdTestCase.java)). ## Changelog [General] [Added] - Add id prop to Text, TouchableWithoutFeedback and View components Pull Request resolved: facebook#34522 Test Plan: Ensure that the new `id` prop android tests pass on CircleCI Reviewed By: lunaleaps Differential Revision: D39089639 Pulled By: cipolleschi fbshipit-source-id: 884fb2461720835ca5048004fa79096dac89c51c
Summary: This PR fixed the `id` prop to being correctly mapped to `nativeID` on the following components: `TouchableBounce`, `TouchableHighlight`, ` TouchableNativeFeedback`, and `TouchableOpacity`. Closes #38117 Follow up of #34522 ## Changelog: [GENERAL] [FIXED] - Fix `id` prop not working on `TouchableBounce`, `TouchableHighlight`, ` TouchableNativeFeedback`, and `TouchableOpacity` Pull Request resolved: #38169 Test Plan: Ensure that the `id` prop android tests pass on CircleCI Reviewed By: jacdebug Differential Revision: D47209319 Pulled By: cortinico fbshipit-source-id: 50cdf0f1113e067aa46d55e4faaff6818509546e
Summary
This adds the
id
prop toText
,TouchableWithoutFeedback
andView
components as requested on #34424 mapping the existingnativeID
prop toid
. As this components are inherited by others this also adds theid
prop support toImage
,TouchableBounce
,TouchableHighlight
,TouchableOpacity
andTextInput
.This PR also adds android tests ensuring that the
id
property is passed to the native view via thenativeID
prop, these tests were based on the existingnativeID
tests (NativeIdTestCase.java).Changelog
[General] [Added] - Add id prop to Text, TouchableWithoutFeedback and View components
Test Plan
Ensure that the new
id
prop android tests pass on CircleCI