Fix/segmentedControl performance #1375
Conversation
| [onChangeIndex, selectedSegment, updateSelectedSegment]); | ||
| [onChangeIndex, updateSelectedSegment]); |
There was a problem hiding this comment.
Are you sure that's correct?
You are using selectedSegment in this callback, removing it can lead to bugs
There was a problem hiding this comment.
You are right, but actually, I think the condition is not required at all..
Fixed :)
There was a problem hiding this comment.
I think the condition is required (:
It meant to prevent calling the onChangeIndex callback in case the user press twice at the same segment.
Now when pressing twice on the same index I still get this callback 😬
There was a problem hiding this comment.
Actually, IMO the user should handle this (maybe the prop name is misleading and onSegmentPress is better).
Because the user might want a callback for any press and we should support it.
Let me know what you think, 'cause I'm not sure what is better :)
There was a problem hiding this comment.
We can add a separate prop for onPress though I'm not sure it's needed.
If you think about it, product wise the user wants to be notified on index change, that's the purpose of the component, to change between different segments.
Our goal is to provide an API that is useful for the user
|
@ethanshar |
| updateSelectedSegment(index); | ||
| setTimeout(() => onChangeIndex?.(index), 400); |
There was a problem hiding this comment.
Other than delaying the callback, I'd consider also to throttle the calls.
For instance, if the user press quickly on multiple different segments, maybe we can throttles all of these calls and trigger only the last one.
Where are you checking the performance of this component? It's mostly noticeable in our SectionWheelPicker example screen.
Another thing worth checking (can be in another PR) is to move to reanimated2, other than simplifying the code it can also help with performance.
There was a problem hiding this comment.
I tried to wrap the callback with lodash throttle but couldn't see any change.. also, I couldn't see the throttle effect in other places we use it (PickerModal for example), let me know if it's only me
I'm checking it in the SectionWheelPickerScreen as well :)
I have a draft PR on moving this component to reanimated2, I'll publish it after this PR :)
Description
Fix segmentedControl performance
Jira WOAUILIB-1853
Changelog
Fix segmentedControl performance