Feat/with scroll enabler #807
Conversation
src/commons/withScrollEnabler.tsx
Outdated
|
|
||
| const scrollViewRef = useRef<WithScrollEnablerProps>(null); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Why do u need an access to the Scroll element ref?
src/commons/withScrollEnabler.tsx
Outdated
| return ( | ||
| <WrappedComponent | ||
| {...props} | ||
| onContentSizeChange={onContentSizeChange} |
There was a problem hiding this comment.
There might be a little risk with passing the Scroll props as is. with the same names.
They can be passed in a spread by mistake to places you don't want. also I think we might want to have a more explicit name for them so the user should understand where they are coming from .
Another option is to group them as an object that the user can spread intentionally
for instance
<WrappedComponent scrollEnablerProps={{onLayout, scrollEnabled, onContentSizeChange}}
There was a problem hiding this comment.
Done, I've created two use classes, 1 for FlatList and 1 for ListView, they are very similar but I'm not sure how easy they are to combine.
There was a problem hiding this comment.
Overall the component looks good.
I noticed that there are some mis-usages of useCallback and in general how you handle function inside a function component
I suggest going over this list of common hooks
https://reactjs.org/docs/hooks-reference.html#useimperativehandle
src/commons/withScrollEnabler.tsx
Outdated
| setContentSize(size); | ||
| } | ||
| }, | ||
| [props] |
There was a problem hiding this comment.
Is there a reason to pass all props in the dependency array?
There was a problem hiding this comment.
Fixed and removed - does not need to use state here
src/commons/withScrollEnabler.tsx
Outdated
| }, | ||
| [props]); | ||
| }, | ||
| [props] |
There was a problem hiding this comment.
same here?
why do u pass all the props?
There was a problem hiding this comment.
Fixed and removed - does not need to use state here
|
|
||
| const WithScrollEnabler = withScrollEnabler( | ||
| (props: WithScrollEnablerProps) => { | ||
| function renderItem(index: number) { |
There was a problem hiding this comment.
You must use useCallback in order to define methods inside a function component.
Otherwise what will happen is that on every render you will re-create those functions.
There was a problem hiding this comment.
Changed to useCallback
|
|
||
| const WithScrollEnabler = withScrollEnabler( | ||
| (props: WithScrollEnablerProps) => { | ||
| const getData = memoize((numberOfItems: number) => { |
There was a problem hiding this comment.
even though you memoize this function you still re-creating it on each render.
There was a problem hiding this comment.
Changed to useCallback
# Conflicts: # generatedTypes/index.d.ts
src/commons/withScrollEnabler.tsx
Outdated
| if (isScrollEnabled !== scrollEnabled) { | ||
| setScrollEnabled(isScrollEnabled); | ||
| } | ||
| }, [contentSize, layoutSize]); | ||
| }, []); |
There was a problem hiding this comment.
If you're using something inside the callback it should be part of the dependencies array of the useCallback
in this case you're using scrollEnabled state, so it should be included in the array.
| @@ -42,7 +42,7 @@ function withScrollEnabler<PROPS extends SupportedViews>( | |||
| } | |||
| } | |||
| }, | |||
| [props.horizontal] | |||
| [props.horizontal, checkScroll] | |||
There was a problem hiding this comment.
I don't think it's recommended to pass to the dependency array a callback.
and actually I don't think you even need to, I think [props.horizontal] is enough in this case.
same goes for the onLayout callback
There was a problem hiding this comment.
That was recommended by the new plugin.
There are still a few TS errors that I'm not sure how to solve.