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.

Sign up for GitHub

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

Jump to bottom

feat: Add readOnly prop to TextInput component #34444

Closed
wants to merge 3 commits into from
Closed

feat: Add readOnly prop to TextInput component #34444

wants to merge 3 commits into from

Conversation

gabrieldonadel
Copy link
Collaborator

Summary

This adds the readOnly prop to TextInput as requested on #34424 mapping the existing editable prop to readOnly so that readOnly={false} maps to editable={true} and readOnly={true} represents editable={false}. This PR also updates the TextInputExample on the RNTest in order to facilitate the manual QA of this.

Changelog

[General] [Added] - Add readOnly prop to TextInput component

Test Plan

  1. Open the RNTester app and navigate to the TextInput page
  2. Test the TextInput component through the Editable and Read only section
Screen.Recording.2022-08-18.at.01.39.04.mov

@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 18, 2022
@gabrieldonadel gabrieldonadel mentioned this pull request Aug 18, 2022
Closed
70 tasks
@gabrieldonadel gabrieldonadel force-pushed the feat/add-read-only-to-text-input branch from 76717f3 to 27c6cc6 Compare August 18, 2022 04:43
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 18, 2022
@gabrieldonadel gabrieldonadel force-pushed the feat/add-read-only-to-text-input branch from 27c6cc6 to a07a65c Compare August 18, 2022 04:49
@analysis-bot
Copy link

analysis-bot commented Aug 18, 2022 •
edited
Loading

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

Base commit: e0a71fc
Branch: main

@motiz88
Copy link
Contributor

motiz88 commented Aug 18, 2022 •
edited
Loading

@necolas do we have an opinion on what should happen when both props are specified (and conflict)?

  • Should the new prop take precedence?
  • Should the old prop take precedence (as here AFAICT)?
  • Should we perhaps use editable={!readOnly && (editable ?? true)}, so the input becomes read-only if either prop is set to the non-default value?

I'm thinking less about cases of users passing both directly, and more about HOCs/wrapper components that hide the use of editable and may now behave slightly surprisingly with readOnly.

motiz88
motiz88 approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Let's wait for an answer re: handling conflicts between readOnly and editable; the current design may already be fine - I just want to make sure we discuss it.

@necolas
Copy link
Contributor

necolas commented Aug 18, 2022 •
edited
Loading

Thanks for this PR! It looks good.

@necolas do we have an opinion on what should happen when both props are specified (and conflict)?

My inclination is to say that the new props should take precedence, but I don't know the details of how the HOC use case could make that problematic.

@gabrieldonadel
Copy link
Collaborator Author

@motiz88 @necolas what do you guys prefer? Should I update the editable logic or keep it as is?

necolas
necolas reviewed Aug 18, 2022
View reviewed changes
Libraries/Components/TextInput/TextInput.js Outdated
@@ -1392,6 +1400,7 @@ const ExportedForwardRef: React.AbstractComponent<
allowFontScaling={allowFontScaling}
rejectResponderTermination={rejectResponderTermination}
underlineColorAndroid={underlineColorAndroid}
editable={!readOnly}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to have readOnly take priority over editable. Thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I've just updated the logic to match this.

@gabrieldonadel gabrieldonadel force-pushed the feat/add-read-only-to-text-input branch from 1284786 to 2017605 Compare August 18, 2022 22:18
necolas
necolas approved these changes Aug 18, 2022
View reviewed changes
Libraries/Components/TextInput/TextInput.js Outdated
* See https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly
* for more details.
*/
readOnly?: ?boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we try to roughly follow alpha-sort for props, but if it's not being flagged by a linter it's not a problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've just updated it to be in the right order

@gabrieldonadel gabrieldonadel force-pushed the feat/add-read-only-to-text-input branch from 05e968e to 21aec01 Compare August 20, 2022 19:16
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,615,705 +469
android hermes armeabi-v7a 7,030,691 +472
android hermes x86 7,916,183 +470
android hermes x86_64 7,890,145 +457
android jsc arm64-v8a 9,494,364 +110
android jsc armeabi-v7a 8,272,227 +113
android jsc x86 9,432,425 +108
android jsc x86_64 10,025,498 +104

Base commit: e0a71fc
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
lunaleaps reviewed Aug 23, 2022
View reviewed changes
Libraries/Components/TextInput/TextInput.js
@@ -1381,6 +1388,8 @@ const ExportedForwardRef: React.AbstractComponent<
allowFontScaling = true,
rejectResponderTermination = true,
underlineColorAndroid = 'transparent',
readOnly,
editable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a deprecation message on editable if it's used cc @cipolleschi. We could do that in a follow-up PR

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 can open a follow-up PR and also update the docs on https://github.com/facebook/react-native-website/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaleaps here is the follow-up PR #34492 and the PR updating the docs facebook/react-native-website#3278

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in de75a7a.

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 Aug 24, 2022
@gabrieldonadel gabrieldonadel deleted the feat/add-read-only-to-text-input branch August 24, 2022 01:57
This was referenced Aug 24, 2022
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunaleaps lunaleaps lunaleaps left review comments

@necolas necolas necolas approved these changes

@motiz88 motiz88 motiz88 approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gabrieldonadel @analysis-bot @motiz88 @necolas @facebook-github-bot @react-native-bot @lunaleaps