Various tweaks for compatibility with react-native-web #3
Conversation
There was a problem hiding this comment.
Hey, thanks for the PR, I'm happy to review it and merge the changes!
Just to be sure, you are aware that this fork is a temporary fix until the original finds a maintainer or an alternative arises? This fork won't be published to npm and can only be added through a git url and commit.
That said the changes look good to me and I trust you've tested on both RN and web. Unfortunately the repo doesn't have CI tests setup so I can't validate the correctness.
| inputProps() { | ||
| let store = {}; | ||
|
|
||
| for (let key in TextInput.propTypes) { | ||
| if ('defaultValue' === key) { | ||
| continue; | ||
| } | ||
|
|
||
| if (key in this.props) { | ||
| store[key] = this.props[key]; | ||
| } | ||
| } | ||
| Object.keys(this.props).forEach((key) => { | ||
| store[key] = this.props[key]; | ||
| }); | ||
|
|
||
| return store; | ||
| } |
There was a problem hiding this comment.
Why not just return this.props at this point? Also unsure if this could trigger warnings / unexpected behaviour if arbitrary props get forwarded to the TextInput component below.
How about adding a whitelist with the supported TextInput props and using it to filter the props?
There was a problem hiding this comment.
Yeah, both good points!
Why not just return this.props at this point?
I opted to retain the loop to indicate where an escape clause for a prop could be easily added in the future, if the previously existing escape clause needed to be added again (e.g. "defaultValue" === key).
Also unsure if this could trigger warnings / unexpected behaviour if arbitrary props get forwarded to the TextInput component below.
True - an oversight on my part as I'm a TypeScript user and rely on TS to warn of invalid props.
How about adding a whitelist with the supported TextInput props and using it to filter the props?
I thought about this and I wasn't sure if the chance of an error in missing out a supported prop (either by mistake or because react-native adds one in the future) was worth the reward. Also, conscious of React Native not supporting prop types any more.
Happy for suggestions on what you think the best approach for this would be if you can think of an alternative to supporting a copied array of supported props.
There was a problem hiding this comment.
Yeah, I'm a TypeScript user myself, so I'm also all in favour of removing the propTypes overhead. The issue however is that the outer component accepts more props than a standard TextInput. Therefore, TypeScript should also accept these additional parameters without emitting a warning either.
The original uses TextInput.propTypes as a whitelist to filter out the TextInput props out from the full TextField props, so that nothing unexpected ends up there. Perhaps a blacklist would be better suited in the future - we already know which props are meant for the outer TextField component (e.g. by checking the propTypes defined above), so removing them should leave us with the props meant for the TextInput.
|
Thanks for responding so quickly!
Thanks for the heads up. A project I've been working on has been using this fork, so, I thought it'd be worth sharing a fix for the web for anyone else using this fork, rather than the project just using my fork. |
Appreciate it! |
Problem
The component won't render on the web because of the references to components'
propTypesfromreact-native. When aliasingreact-native-webtoreact-native, the attempt to referencepropTypeswill fail becausereact-native-webdoesn't exportpropTypesfrom its equivalent components.Solution
react-nativecomponentpropTypestoPropTypes.any.propTypesare deprecated inreact-native: Deprecate and eventually remove propTypes from core react-native-community/discussions-and-proposals#29Other changes
Labelwhen the text field is focussed (the previous positioning styles in combination with the css translation didn't work on the web). The new solution produces the same result on iOS and the web