feat: fix scroll behavior on asset lists [PERA-3277]#394
feat: fix scroll behavior on asset lists [PERA-3277]#394
Conversation
| const renderHeaderNode = ( | ||
| component: React.ComponentType | React.ReactElement | null | undefined, | ||
| ): React.ReactNode => { | ||
| if (component == null) { | ||
| return null | ||
| } | ||
| if (typeof component === 'function') { | ||
| const Component = component | ||
| return <Component /> | ||
| } | ||
| return component | ||
| } |
There was a problem hiding this comment.
nit: I don't like much this pattern because this is basically returning a React node every time, which means it's a component. But it's not written as a component. And since it's not a component, you cannot use react features which might be interesting sometimes like React.memo, etc.
I'd rather design that as a sub-component instead and use it in the more React idiomatic way.
But there's no issue here since you already mitigated the risks of recalculating the result unnecessarily with the useMemo down the line. So I'm really just expressing an personal opinion.
There was a problem hiding this comment.
I think this is required because of the way the type for this prop is defined on LegendList - it's not really up to us.
|
|
||
| type UseSearchableListParams<T> = { | ||
| forwardedRef: React.ForwardedRef<PWFlatListRef> | ||
| data: readonly T[] | null | undefined |
There was a problem hiding this comment.
maybe?
| data: readonly T[] | null | undefined | |
| data: readonly Maybe<T[]> |
There was a problem hiding this comment.
We have to leave it as is on this occasion because it must be readonly to comply with the PWFlatList prop requirements (inherited from LegendList) and you can't make a custom type readonly
| type UseSearchableListParams<T> = { | ||
| forwardedRef: React.ForwardedRef<PWFlatListRef> | ||
| data: readonly T[] | null | undefined | ||
| keyExtractor?: (item: T, index: number) => string |
There was a problem hiding this comment.
What do you think about using the Maybe type in such cases as well just for the sake of consistency?
| keyExtractor?: (item: T, index: number) => string | |
| keyExtractor?: Maybe<(item: T, index: number) => string> |
There was a problem hiding this comment.
I think the ? optionality already covers what we need, no? Do we need to double that up with a type wrapper too?
| // Defer the scroll so it runs after FlashList re-renders with the | ||
| // shrunken dataset; scrolling synchronously while cells are being | ||
| // recycled produces a jittery animation. | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
I think this serves the exact same purpose as the deferToNextCycle function we have in the shared package, right? Should we do something about it?
There was a problem hiding this comment.
They are subtly different things I think. setTimeout(0) (which is what deferToNextCycle) will add the funciton call to the end of the event loop, so it will do all other async/await stuff already queued up first, then this thing.
requestAnimationFrame will defer to the next render cycle (i.e. run on the next screen refresh). Because we're trying to prevent animation jitter here, I think the animation hook is more appropriate.
2 participants
Pull Request Template
Description
This PR fixes 4 issues:
Related Issues
Closes https://algorandfoundation.atlassian.net/browse/PERA-3277
Checklist
Additional Notes