feat: support 'editable' prop on TextInput #517
Conversation
src/fireEvent.js
Outdated
| */ | ||
| // eslint-disable-next-line import/no-extraneous-dependencies | ||
| const { TextInput } = require('react-native'); | ||
| return element && element.type === TextInput; |
There was a problem hiding this comment.
nit pick but wouldn't this work here too return element?.type === TextInput
There was a problem hiding this comment.
also, I like your first proposal. I'm not sure if proposal 2 is necessary but I'll wait to hear what someone else has to say.
There was a problem hiding this comment.
Let's go with 1 for now, we do something similar in src/helpers/getByAPI.js
There was a problem hiding this comment.
Thanks for the catch @DallasCarraher!
Went with the 1st approach as suggested.
6cbc3c07a509f1c0f0754e83129e81a30268a87
src/__tests__/fireEvent.test.js
Outdated
| return <TextInput placeholder={placeholder} onChangeText={onChangeText} />; | ||
| }; | ||
|
|
||
| test('should not be fooled by non-native editable prop', () => { |
There was a problem hiding this comment.
Sorry maybe I'm naïve or missing something here so I apologize if this is a silly question, but what is this testing? The editable prop isn't being spread / drilled down in your TextInputWrapper so it shouldn't affect the nested TextInput anyway right?
There was a problem hiding this comment.
Hm I added this test since we have a similar one for TouchableOpacity above it, but it seems like it is not necessary in the case of TextInput where we don't care about the event propagating to the custom parent component. I'll remove it thanks!
5f861da352b569c124db17b3f93c5e63dd0ccf77
src/fireEvent.js
Outdated
| @@ -12,10 +34,14 @@ const findEventHandler = ( | |||
| const handler = getEventHandler(element, eventName); | |||
| const hasHandler = handler != null || hasDescendandHandler; | |||
|
|
|||
| const isHostComponent = typeof element.type === 'string'; | |||
| const isHostComponent = | |||
| typeof element.type === 'string' || isTextInputComponent(element); | |||
There was a problem hiding this comment.
please revert this check. host components are always of string type
There was a problem hiding this comment.
74b8c8a28e8957324404b77615288060fc34f9fc
src/__tests__/fireEvent.test.js
Outdated
| expect(onChangeTextMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('should not fire on disabled TextInput with nested Text', () => { |
There was a problem hiding this comment.
There's no need for this test, can we remove it?
There was a problem hiding this comment.
Just to clarify, this test is also mentioned in #517 (comment).
Should we keep it therefore or remove it?
| const NEW_TEXT = 'New text'; | ||
|
|
||
| const { getByPlaceholderText } = render( | ||
| <View> |
There was a problem hiding this comment.
we could add the custom TextInputWrapper here and combine all cases into a single test :)
pplytas
force-pushed
the
feature/support-editable-on-textinput
branch
from
78c1e82 to
95b397a
Compare
|
Squashed the fixups. Ready to go! |
Summary
Prevent
fireEventfrom calling event handlers onTextInputwheneditableprop isfalse.Fixes #476
Implementation:
Added
isTextInputComponent()helper function to check if element is aTextInputand if it is then checkeditableprop instead ofonStartShouldSetResponderTest plan
Added tests for standalone TextInput and TextInput with a nested Text component.