Picker Refactor #1 #1000
Picker Refactor #1 #1000
Conversation
src/components/picker/PickerItem.js
Outdated
| } = this.props; | ||
| }; | ||
|
|
||
| const renderSelectedIndicator = () => { |
There was a problem hiding this comment.
maybe it's better to useMemo in here
| const {disabled} = this.props; | ||
| }; | ||
|
|
||
| const _renderItem = () => { |
There was a problem hiding this comment.
also in here, should be memoized
There was a problem hiding this comment.
I don't want to overkill the component with memoization.
I read in a blog post about when to useMemo and it says it's not recommended to overuse memoization cause it the it can also affect performance.
From what I read, the thumb rule is to start simple and to consider memoizing when we face performance issues.
memoizing this will require wrapping getLabel with useCallback and I prefer not to complicate it at the moment.
Anyway, Personally I still not certain what is best practice with how often to use useMemo so again, for now I prefer to go easy on it..
There was a problem hiding this comment.
Gotcha. confusing topic indeed :)
| onSelectedLayout: PropTypes.func | ||
| }; | ||
|
|
||
| export default PickerItem; |
There was a problem hiding this comment.
PickerItem should be exported as memoized component
There was a problem hiding this comment.
Due tho some of the of the props in PickerItem (like renderItem callback prop), it's very possible that memoizing the whole component might cause some side effects and break behaviors.
For now I think it will be better to keep parity with the previous implementation (where PickerItem was simple Component and not a pure one) and later on consider optimizations if necessary.
| const {value, label} = this.props; | ||
| const onSelectedLayout = useCallback((...args) => { | ||
| _.invoke(props, 'onSelectedLayout', ...args); | ||
| }, []); |
There was a problem hiding this comment.
shouldn't we pass props as a dependency?
There was a problem hiding this comment.
this is internal usage.
onSelectedLayout is being sent from the Picker and it doesn't change..
we need it for first mount, so dependencies are irrelevant in this case..
| generateStyles() { | ||
| this.styles = createStyles(this.props); | ||
| } | ||
| // TODO: deprecate the check for object |
There was a problem hiding this comment.
TODO hints for isObject check, but in onPress it's commented out
There was a problem hiding this comment.
I removed it in a following PR
src/components/picker/PickerItem.js
Outdated
| } | ||
| } | ||
|
|
||
| }, []); |
There was a problem hiding this comment.
There is a lint error,
We should either pass value as a dependency or no square brackets to trigger on each render
mendyEdri
left a comment
mendyEdri
left a comment
There was a problem hiding this comment.
I've learned few things from your comments 👏
Description
Step 1 - Migrate PickerItem from Class to a Function component.
This in order to start using Context for communicating between the Picker parent component and its children (PickerItems)
Once PickerItem is a Function component I'll be able to use hooks and specifically the
useContexthook.Changelog
Refactor PicketItem component