[SliderIOS] Add option for both min/max track image.

[christopherdro]

This is a followup to PR #3850 but now separates min/max track images into different properties.
Closes #4476

Add examples for minimumTrackTintColor, maximumTrackTintColor, minimumTrackImage, maximumTrackImage to UIExplorer.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @manask88, @vjeux and @nicklockwood to be potential reviewers.

[rclai]

Oh dude! Thanks!

[ide]

For backwards compatibility it might be prudent to keep support for trackImage, and have it act as the default value the two min/maxTrackImage props when they are not specified.

[ide]

Also, on naming: do we have a convention for using "minimum" and "maximum" or do we abbreviate to "min" and "max"?

[nicklockwood]

@ide +1 for keeping trackImage for backwards compatibility.

Re: min/max, I think the convention for iOS-specific controls or properties is to stick with the UIKit name unless there's an existing web standard for the same concept. That may lead to inconsistencies though, as UIKit tends to use very verbose names and the web tends to be more terse.

[facebook-github-bot]

@christopherdro updated the pull request.

[facebook-github-bot]

@christopherdro updated the pull request.

[christopherdro]

Just updated backwards compatibility with trackImage.

Now, I'm thinking it makes sense to allow a single tintColor option that will set both minimumTrackTintColor & minimumTrackTintColor. Thoughts?

[nicklockwood]

@christopherdro I just tried it, and it looks like the standard iOS UISlider behavior is to apply the inherited tintColor (i.e. the tintColor of the window or parent view) only on the left hand side (equivalent to setting minimumTrackTintColor explicitly), but setting tintColor on the slider itself does nothing.

Since the iOS style convention is that only the left side is colored, and the right is gray, I'd avoid adding a new tintColor property to the control that a) doesn't match iOS behavior, and b) encourages people to style the control in a nonstandard way.

Calling the property trackTintColor would be slightly better, but still falls foul of point b) above, so I'd probably just leave it.

[nicklockwood]

FYI, I think the comment that the image must be stored in xcassets is incorrect. The image must be static, but it should work for any image file inside the application bundle, including ones bundled using the new image packaging system that @frantic added (where images are placed relative to the js files, and referenced with require('./image.png'). If that's not the case, it's probably a bug we should investigate.

[christopherdro]

@nicklockwood I can confirm that using the new image packaging system does not work.
Error setting property 'minimumTrackImage' of RCTSlider with tag #99: JSON value '1' of type NSNumber cannot be converted to an image

Will try and take a closer look.

[christopherdro]

This might be specific to UIExplore since image build script is missing.

Although the inline image example from <Text> uses <Image source={require('./flux.png')}/>. So im not sure.

One thing that I noticed is that I can reference relative image files by somewhat sticking to the old syntax of require(image!bunny)

Update: Adding a script to run react-native-xcode.sh seems like it won't work anyway without altering it to work with the way UIExplorer is setup.

[nicklockwood]

Ah, I think the problem is that we need to call resolveAssetSource() on all of the image props on the JS side before passing to native.

[christopherdro]

Yea it's missing. Will get that implemented for this PR as well

[christopherdro]

@nicklockwood Images with the new asset system should be working properly now.

We should also make minimumTintColor and maximumTintColor consistent with other components and use processColor before passing them into the the native component.

However, this brings up issue #4547 that I opened up a few days ago.

[facebook-github-bot]

@christopherdro updated the pull request.

[rclai]

Question, how do we account for the cap insets of the left and right? For example, if I have a minimum track image that has a left cap curve, there's going to be an unwanted stretching behavior unless the image has the cap insets defined in order to let iOS know which part of the image to tile and leave the rest unstretched.

Check out the description about the image:
https://developer.apple.com/library/prerelease/ios/documentation/UIKit/Reference/UISlider_Class/index.html#//apple_ref/occ/instm/UISlider/setMinimumTrackImage:forState:

Defining the cap insets of the image:
https://developer.apple.com/library/prerelease/ios/documentation/UIKit/Reference/UIImage_Class/#//apple_ref/occ/instm/UIImage/resizableImageWithCapInsets:

Perhaps we can introduce a minimum/maximumTrackImageCapInset prop, which takes a plain object of this form { left: 20, right: 20, top: 0, bottom: 0 }, that can be converted to a struct to pass to resizableImageWithCapInsets.

[nicklockwood]

I'm thinking that maybe capInsets should be defined in the imageSource object, along with width/height/scale.

[rclai]

I just found out this API exists for Image components: https://facebook.github.io/react-native/docs/image.html#capinsets but don't know if there is overlap for implementation here.

[nicklockwood]

@rclai yeah, we support CapInsets on <Image>, but that's not really useful here unless we require you to pass an entire image component as the slider background, which seems a bit strange.

[christopherdro]

I plan on taking a further look at this shortly. Just caught up with another issue at the moment.

[christopherdro]

I appears that the bundler handles determining the image size and scale whereas capInsets are going to be user defined.

I've already tried using the original implementation of setting the UIEdgeInsets based on the width of the image, but provides a undesirable effect. See Demo

I don't really see a way without altering resolveAssetSource and RCTConvert and having to the capInsets for each image.

@nicklockwood Did you already have an idea of how this could be implemented?

@rclai Could you provide some more information what your trying to achieve? Maybe some examples of the images?

[rclai]

@christopherdro this tutorial is a perfect example of what I'm talking about: http://www.raywenderlich.com/32167/photoshop-tutorial-for-developers-creating-a-custom-uislider

Notice the left and right curved caps, then at the bottom, you'll find the code needed to get make sure those caps are not stretched and the rectangle defined for tiling.

PS - I'm not able to see the image you posted.

[nicklockwood]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/440883629449664/int_phab to review.

[nicklockwood]

I think I've found a good solution to the scalable image problem - at least, it allows you to specify track images with stretchable centers and fixed end-caps, which I think is the common case. For more complex patterns like zebra stripes we would need to support tiling, which would mean exposing additional configuration options, but that can be a job for another PR.

[christopherdro]

@nicklockwood Sounds like a good solution.

Do you think I should update the PR to use processColor for the tint color options or have that be separate?

[nicklockwood]

No need. Both colors and images are now handled automatically by the requireNativeComponent logic.

[christopherdro]

👍
Should we update the prop type to accept objects as well or just stick with strings?

[nicklockwood]

I'll figure out a proper solution for the color proptype warnings. It's silly to have to solve this individually in each place it's used.

[rclai]

Thanks guys.