Skip to content

fix(Slider): Fix issues with scrub and allowTouchTrack - #3203

Merged
arpitBhalla merged 9 commits into
react-native-elements:nextfrom
BioTrakHealth:slider-scrub-fix
Jan 11, 2022
Merged

fix(Slider): Fix issues with scrub and allowTouchTrack#3203
arpitBhalla merged 9 commits into
react-native-elements:nextfrom
BioTrakHealth:slider-scrub-fix

Conversation

@CodeToTee

Copy link
Copy Markdown
Contributor

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, and onSlidingComplete callback 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):

  • Added 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.
  • Moved function getBoundedValue(), handlePanResponderRequestEnd(), and handleMoveShouldSetPanResponder() out of the component, as they're not dependent on any component state variables.
  • Added SizableVars enum to describe possible vars managed within handleMeasure()
  • Extracted setting allMeasured into a stand-alone useEffect() so it gets automatically updated whenever any of the SizableVars structures change.
  • Destructured all props used within the FC so they're not passed-down unnecessarily (otherwise they get passed into the container View).
  • Removed didMountRef as it really wasn't needed.
  • Remove numerous cases where an optional prop was used as someProp || defaultValue, as that's not needed because in those cases the defaultProps ensure default values exist at initial render.
  • Always range-bound props.value at the top of the function. This eliminated the need to bound it later (as was done multiple times).
  • Changed isVertical from ref to calculated value, as it will now update if props are changed.
  • Review @menssen comments re his changes; those are mostly included in this PR.

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.

…, 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
@netlify

netlify Bot commented Sep 16, 2021

Copy link
Copy Markdown

✔️ 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
@CodeToTee

Copy link
Copy Markdown
Contributor Author

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!

@freemanUA

Copy link
Copy Markdown

is this alive?

@CodeToTee

Copy link
Copy Markdown
Contributor Author

I've never received any feedback from the maintainers re this PR. I'm happy to update as needed, but I have no feedback.

@arpitBhalla

Copy link
Copy Markdown
Member

Could you please resolve conflicts.

@CodeToTee

CodeToTee commented Jan 10, 2022

Copy link
Copy Markdown
Contributor Author

Done. I simply merged the latest 'next' branch. There were no conflicts. Thanks.

@arpitBhalla

Copy link
Copy Markdown
Member

The changes looks good to me, Thanks for great work @NiwotSmitty ;)

@arpitBhalla
arpitBhalla requested a review from khushal87 January 10, 2022 19:56
@arpitBhalla
arpitBhalla merged commit 74de9ea into react-native-elements:next Jan 11, 2022
@CodeToTee
CodeToTee deleted the slider-scrub-fix branch January 11, 2022 14:26
@Ashutosh-Tiwari

Copy link
Copy Markdown

@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.

@KrisLau

KrisLau commented Jun 24, 2022

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slider allowTouchTrack prop is not working Slider props value changes not rendering correctly

5 participants