Migrate Picker implementation to TS and use Function component #1870
Conversation
| LogService.warn('Picker in MULTI mode must accept an array for value'); | ||
| } | ||
|
|
||
| // TODO: this warning should be replaced by the opposite |
There was a problem hiding this comment.
This TODO is confusing, please leave only the relevant warning and rewrite the comment if still relevant
| // TODO: deprecate getItemLabel prop | ||
| // TODO: Add initialValue prop | ||
| // TODO: consider deprecating renderCustomModal prop | ||
| // TODO: deprecate onShow cause it's already supported by passing it in pickerModalProps |
There was a problem hiding this comment.
- Why change
_.isPlainObjectusage? getItemValueandgetItemLabeldeprecation started, so we can remove comments.- Why not start with the deprecation of
onShow?
There was a problem hiding this comment.
- For TS, when using comparison like this (
typeof value === 'object'), Typescript actually interpret the right type of thevalueprop. When using lodash unfortunately it doesn't. - They didn't really started, I just added the @deprecated annotation long time ago. We need to address it accordingly.
- That's not the purpose of this PR, notice that most of the TODOs here are old.
The main purpose of this PR is migrate to TS and function component, which is big as it is, I didn't want to add more stuff.
| @@ -138,7 +138,7 @@ export default { | |||
| return require('./components/panningViews/panResponderView').default; | |||
| }, | |||
| get Picker() { | |||
| return require('./components/picker').default; | |||
| return require('./components/picker/new').default; | |||
There was a problem hiding this comment.
Where are you routing to the old picker?
There was a problem hiding this comment.
I'm not, it's routing to the new
There was a problem hiding this comment.
So you're keeping the old files as a reference?
There was a problem hiding this comment.
Yes, and in case we'll have a big issue we can quickly use the old file without reverting the whole PR
After some time I'll just remove it
There was a problem hiding this comment.
Ok. I suggest opening tickets for both completing this migration (and deleting old files) and starting the props migrations needed.
|
@ethanshar Looks good. I had only small comments. Also, please fix example screens, there are examples with old API that produce warnings. |
The example still have old usages so we'll know we didn't break anything while making changes, in the end of the screen there's an example with a migrated usage |
Description
Refactor the Picker component
newfile) and the internal sub components/filestypesfileIf you prefer to review it together cause it's a little overwhelming let me know.
Changelog
Refactor Picker component and convert to TS