fix(Slider): Fix issues with scrub and allowTouchTrack - #3203
Conversation
…, and onSlidingComplete - refactor to use useCallback rather than funcitons generated on every render - fix test cases - add new test cases for slightly better code coverage
This reverts commit ffca1af. # Conflicts: # dist/Slider/Slider.js
|
✔️ Deploy Preview for react-native-elements canceled. 🔨 Explore the source changes: 30125bc 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-native-elements/deploys/61dc8d10c57ff70007939e9a |
- gesture handlers weren't receiving proper dx/dy values, so updated to save x0/y0 at start of gesture - added default prop values to destructing at function entry
|
Any comments/feedback on this PR? I added a couple fixes to the PR. I've not contributed before, so the workflows aren't running. If there is something wrong with this PR, please let me know and I'll address it. Thank you! |
|
is this alive? |
|
I've never received any feedback from the maintainers re this PR. I'm happy to update as needed, but I have no feedback. |
|
Could you please resolve conflicts. |
|
Done. I simply merged the latest 'next' branch. There were no conflicts. Thanks. |
|
The changes looks good to me, Thanks for great work @NiwotSmitty ;) |
|
@arpitBhalla In which version of react-native elements will this fix be available? I am using 3.4.2 currently and I still don't see it fixed in my case. |
|
Omg I just saw this I thought it was my fault. I kept adding random things like dynamic keys to try and force a re-render. Thanks so much for this. |
What kind of change does this PR introduce?
Fixes #3087 and #3130
Did you add tests for your changes?
Yes, existing test cases were fixed, snapshots updated, and new test cases added.
Summary
This PR is a significant refactor of the Slider component. The primary driver of this change is to fix the
onValueChange,onSlidingStart, andonSlidingCompletecallback props, as they weren't sending in the current position. I believe they were broken when the Slider component was ported to a function component.Before starting this update, I also realized the test scripts were failing on the Slider component. So, this PR also fixes the tests, updates the snapshots, and adds a couple new test. Unfortunately, the test coverage is lower than I'd like, but the primary reason is the Pan Gesture handlers, as they're commonly cumbersome and tedious to test with Jest.
Additionally, major kudos to @menssen for proving a great outline of what needed to be done, and providing a patch. As @menssen pointed out in his comments, his changes had been de-typescripted, so I use his changes as a starting point, added-in typescript where appropriate, and fixed/optimized other items as I came across them..
Here are some notes about the changes (again, thanks to @menssen for a fantastic starting point):
useCallback()to functions within the component, and added the appropriate dependencies array. Note this change required reordering of many of these functions to eliminate compiler complaints. Unfortunately, this makes the diff a little harder to follow.getBoundedValue(),handlePanResponderRequestEnd(), andhandleMoveShouldSetPanResponder()out of the component, as they're not dependent on any component state variables.SizableVarsenum to describe possible vars managed withinhandleMeasure()allMeasuredinto a stand-aloneuseEffect()so it gets automatically updated whenever any of theSizableVarsstructures change.didMountRefas it really wasn't needed.someProp || defaultValue, as that's not needed because in those cases thedefaultPropsensure default values exist at initial render.props.valueat the top of the function. This eliminated the need to bound it later (as was done multiple times).isVerticalfrom ref to calculated value, as it will now update if props are changed.I think that captures the gist of the changes. I realize this is a large PR, so thanks in advance for review. IMO, this is a pretty important PR, as the existing implementation doesn't work in the 3.x stream. In my app, I had to remain on the 2.3 stream until. I fixed these issues.
Please let me know if there are any questions or issues.