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

Jump to bottom

fix: expose ref in text input #1218

Merged
merged 3 commits into from Aug 23, 2019
Merged

fix: expose ref in text input #1218

merged 3 commits into from Aug 23, 2019

Conversation

jaulz
Copy link
Collaborator

@jaulz jaulz commented Jul 16, 2019

Motivation

Currently, it is not possible to pass a ref prop to the TextInput.

Test plan

Use TextInput with ref prop.

@jaulz jaulz force-pushed the jaulz/fix-expose-ref-in-textinput branch from fb1939b to 5821a30 Compare July 16, 2019 09:52
@callstack-bot
Copy link

callstack-bot commented Jul 16, 2019 •
edited

Hey @jaulz, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

ferrannp
ferrannp reviewed Jul 19, 2019
View reviewed changes
src/components/TextInput/TextInput.tsx
@@ -395,7 +397,8 @@ class TextInput extends React.Component<TextInputProps, State> {
}

render() {
const { mode, padding, ...rest } = this.props;
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { mode, padding, ref: outerRef, ...rest } = this.props;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do you use outerRef ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nowhere but we need to remove it from rest since we cannot pass it on to the inner components.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But then how we are able to pass our own ref to TextInput and use 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.

The ref will be passed to TextInput and React will take care of it so you can access the methods of it, e.g.

react-native-paper/src/components/TextInput/TextInput.tsx

Lines 388 to 390 in 29f3ffc

focus() {
return this._root && this._root.focus();
}

_root again will be set by these piece of codes:

react-native-paper/src/components/TextInput/TextInput.tsx

Lines 407 to 409 in 29f3ffc

innerRef={ref => {
this._root = ref;
}}

react-native-paper/src/components/TextInput/TextInput.tsx

Lines 419 to 421 in 29f3ffc

innerRef={ref => {
this._root = ref;
}}

I didn't want to break anything with the innerRef so that was the simplest solution I think.

@jaulz jaulz requested a review from ferrannp August 13, 2019 12:52
@Trancever
Copy link
Contributor

@jaulz Can you describe what do you want to achieve? Why do you need ref to the React-Native's TextInput.

@jaulz
Copy link
Collaborator Author

jaulz commented Aug 22, 2019

@Trancever at the moment you will see the following error in case you use TypeScript:

Property 'ref' does not exist on type 'IntrinsicAttributes & Pick<TextInputProps, "label" | "style" | "hitSlop" | "onLayout" | "pointerEvents" | "removeClippedSubviews" | "testID" | "nativeID" | "collapsable" | ... 99 more ... | "dense"> & { ...; } & { ...; }'.ts(2322)

You can of course still pass a ref property and it works but it will cause a warning.

This PR will expose the ref property using React.ComponentPropsWithRef here: https://github.com/callstack/react-native-paper/pull/1218/files#diff-3a9942f56421e77a4b6910cb9cac8381R18

Since this means that ref is part of the props we need to explicitly exclude it here: https://github.com/callstack/react-native-paper/pull/1218/files#diff-3a9942f56421e77a4b6910cb9cac8381R401

@Trancever
Copy link
Contributor

Ahh so it's only to fix typescript right? I think we can accept that

@jaulz
Copy link
Collaborator Author

jaulz commented Aug 22, 2019

Sorry for the confusion 😇

@Trancever Trancever merged commit 52b2756 into master Aug 23, 2019
RichardLindhout added a commit to RichardLindhout/react-native-paper that referenced this pull request Aug 28, 2019
@RichardLindhout
Change to material community icon
70e3293 
[fix] added good close icon

[fix] runned yarn and regenerated snapshots

Delete ActivityIndicator.test.tsx

Delete AppbarHeader.test.tsx

fix: only start/stop progressBar animation if visible prop changes (callstack#1294)

docs: fix Surface component docs (callstack#1253)

fix: fix typescript error when setting ref on textinput (callstack#1218)

fix: FAB - always display activity indicator if loading property is set to true (#11570)

feat: (title|description)numberOfLines props to ListItem and ListAccordion (callstack#1245)
@francisco-mercury francisco-mercury mentioned this pull request Sep 2, 2019
Closed
@jaulz jaulz deleted the jaulz/fix-expose-ref-in-textinput branch October 3, 2019 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trancever Trancever Trancever approved these changes

@ferrannp ferrannp Awaiting requested review from ferrannp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jaulz @callstack-bot @Trancever @ferrannp