Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SliderIOS] Apply value after minimum/maximumValue in order to ensure it is properly set #835

Closed
wants to merge 1 commit into from

Conversation

brentvatne
Copy link
Collaborator

value is clamped between min/max and so order of prop application matters - value always ended up being set first in my tests, and consequently a value outside of the default range 0-1 would not work. So this applies the value when the min/max are set.

Gist of broken example

screenshot
^ the second slider here should have it's cursor in the middle

/cc @tadeuzagallo

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 13, 2015
@brentvatne
Copy link
Collaborator Author

While this solution works, I do wonder if it's worth adding a hook on view manager classes that allows us to specify in what order props should be applied, either in JS or Objective-C. eg: instead of having RCTSetViewProps [iterate over each prop in an unreliable order as provided by NSDictionary] the manager class could optionally expose a function that specifies order which RCTSetViewProps can then use to sort.

// RCTSliderManager.m

- (NSArray *)applyPropsFirst
{
  return @[ @"minimumValue", @"maximumValue" ];
}

or...

// RCTSliderManager.m

- (NSArray *)propsOrder
{
  return @[ @"minimumValue", @"maximumValue", @"value" ];
}

This would have been helpful for me on react-native-overlay because I need to ensure visible is set before setting the windowLevel here and on react-native-video because most things are dependent on the src being set.

@tadeuzagallo - thoughts? cc @vjeux

- `value` is clamped between min/max and so order of prop application
  matters
@brentvatne brentvatne changed the title Apply value after minimum/maximumValue in order to ensure it is applied [SliderIOS] Apply value after minimum/maximumValue in order to ensure it is properly set Apr 14, 2015
@brentvatne
Copy link
Collaborator Author

ping @tadeuzagallo - simple change here that we discussed in IRC, would be good to get it into next sync if possible

@nicklockwood
Copy link
Contributor

I've been trying to land this today, but ran into an unexpected merge conflict. Should be sorted shortly.

michalchudziak pushed a commit to michalchudziak/react-native-slider that referenced this pull request Feb 8, 2019
… it is properly set

Summary:
`value` is clamped between min/max and so order of prop application matters - `value` always ended up being set first in my tests, and consequently a value outside of the default range 0-1 would not work. So this applies the value when the min/max are set.

[Gist of broken example](https://gist.github.com/brentvatne/fc637b3e21d012966f3a)

![screenshot](http://url.brentvatne.ca/SQPC.png)
^ the second slider here should have it's cursor in the middle

/cc @tadeuzagallo
Closes facebook/react-native#835
Github Author: Brent Vatne <brent.vatne@madriska.com>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
acoates-ms added a commit to acoates-ms/react-native that referenced this pull request Oct 26, 2021
james-watkin added a commit to james-watkin/react-native-slider that referenced this pull request Dec 28, 2021
… it is properly set

Summary:
`value` is clamped between min/max and so order of prop application matters - `value` always ended up being set first in my tests, and consequently a value outside of the default range 0-1 would not work. So this applies the value when the min/max are set.

[Gist of broken example](https://gist.github.com/brentvatne/fc637b3e21d012966f3a)

![screenshot](http://url.brentvatne.ca/SQPC.png)
^ the second slider here should have it's cursor in the middle

/cc @tadeuzagallo
Closes facebook/react-native#835
Github Author: Brent Vatne <brent.vatne@madriska.com>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants