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

feat: Add id prop to Text, TouchableWithoutFeedback and View components #34522

Closed

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Aug 28, 2022

Summary

This adds the id prop to Text, TouchableWithoutFeedback and View components as requested on #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).

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

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 28, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 28, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 28, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,641,920 -7
android hermes armeabi-v7a 7,054,280 +0
android hermes x86 7,943,689 -5
android hermes x86_64 7,915,598 -7
android jsc arm64-v8a 9,515,616 +75
android jsc armeabi-v7a 8,291,238 +85
android jsc x86 9,454,950 +80
android jsc x86_64 10,046,016 +79

Base commit: e8739e9
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 28, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 73abcba
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@lunaleaps
Copy link
Contributor

lunaleaps commented Aug 29, 2022

@gabrieldonadel Wow you're on fire with all these PRs! 👏
Can we also add a PR for documentation for this new property and also a PR for https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-native? We probably won't be able to merge (since this would target 0.71 and DT doesn't have a package for main) -- but we have plans to move those types into this repo and it would help me track the change. Let me know if you need help with that!

@gabrieldonadel
Copy link
Collaborator Author

@gabrieldonadel Wow you're on fire with all these PRs! 👏 Can we also add a PR for documentation for this new property and also a PR for https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-native? We probably won't be able to merge (since this would target 0.71 and DT doesn't have a package for main) -- but we have plans to move those types into this repo and it would help me track the change. Let me know if you need help with that!

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)

@gabrieldonadel gabrieldonadel force-pushed the feat/add-id-to-view branch 2 times, most recently from 4609f1b to f4a9901 Compare August 30, 2022 04:00
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@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,
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

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.

@gabrieldonadel gabrieldonadel force-pushed the feat/add-id-to-view branch 2 times, most recently from 06045fd to 863a4a2 Compare September 5, 2022 17:06
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel gabrieldonadel force-pushed the feat/add-id-to-view branch 3 times, most recently from d818f94 to 76a7beb Compare September 8, 2022 17:50
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in 3e97d5f.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 9, 2022
@gabrieldonadel gabrieldonadel deleted the feat/add-id-to-view branch September 9, 2022 18:31
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…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
facebook-github-bot pushed a commit that referenced this pull request Jul 4, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants