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

Make AnimatableSlider inherit from UISlider and add designable images #417

Merged
merged 2 commits into from Mar 27, 2017

Conversation

phimage
Copy link
Member

@phimage phimage commented Mar 25, 2017

AnimatableSlider do not extends UISlider, only UIView

thumb, minimum and maximum track images added has designable objects in storyboard

PS:
I try some experiment on slider size by overriding trackRect()

 @IBInspectable var trackHeight: CGFloat = CGFloat.nan
    
    override func trackRect(forBounds bounds: CGRect) -> CGRect {
       // add here if not trackHeight.isNan
        return CGRect(origin: bounds.origin, size: CGSize(width:bounds.width, height:trackHeight))
    }
 // or
    override func trackRect(forBounds bounds: CGRect) -> CGRect {
       let rect = super.trackRect(forBounds: bounds)
       return ...
    }

There is some bug when using maximum and minimum images
to fix some other bugs in storyboard I use in trackRect()


#if TARGET_INTERFACE_BUILDER
 // do some stuff to adjust storyboard
#endif

@IBAnimatableBot
Copy link

IBAnimatableBot commented Mar 25, 2017

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 Danger

Copy link
Member

@JakeLin JakeLin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like you PR very much with a lot of documentation comments 👍👍👍. I think this PR is ready to go. It will be greate if you can modify the CHANGELOG.md file. It is very simple like https://github.com/IBAnimatable/IBAnimatable/blob/master/CHANGELOG.md#enhancements

One thing I need to confirm with @tbaranes on why did we inherit from UIView. If he is OK, we will merge it. Thanks again 💪


}

extension SliderImagesDesignable where Self: UISlider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make it public for the others can use those extension methods outside of IBAnimatable framework.

extension SliderImagesDesignable where Self: UISlider {

func configureThumbImage() {
self.setThumbImage(thumbImage, for: .normal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we remove all self?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, also all the others below 😬

@JakeLin
Copy link
Member

JakeLin commented Mar 26, 2017

@phimage thank you very much for your PR. Please have a look at my small comments.

@tbaranes In #228, we created AnimatableSlider. I know we want to have fully customizable AnimatableSlider and UISlider can't give us that 😕. Therefore, we let it inherit from UIView instead of UISlider. @phimage found out that and made AnimatableSlider to inherit from UISlider. I think that is better for now since the current AnimatableSlider is more like AnimatableView.

If we want to have fully customizable AnimatableSlider in the future, we may create another class like AnimatableCustomSlider to inherit from UIView. Can I have your opinion on this PR? thanks.

extension SliderImagesDesignable where Self: UISlider {

func configureThumbImage() {
self.setThumbImage(thumbImage, for: .normal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, also all the others below 😬

@tbaranes
Copy link
Member

I agree with you @JakeLin, that PR is a good move since it's making AnimatableSlider more slider. I don't really remember why we choose UIView instead of UISlider in #228, but I can't see any reasons. Truth is when we will do the custom sliders, it won't really be related to AnimatableSlider, so... who cares?

@phimage Good job on this!

@phimage
Copy link
Member Author

phimage commented Mar 26, 2017

Ok for the changes (public and self)
For changelog I make two lines? one in "api breaking changes" or not?

I will fix also some changelog issue with markdown comma

@phimage phimage changed the title Fix AnimatableSlider parent class and add designable images Make AnimatableSlider inherit from UISlider and add designable images Mar 26, 2017
@JakeLin JakeLin merged commit cc0c27d into IBAnimatable:master Mar 27, 2017
@JakeLin
Copy link
Member

JakeLin commented Mar 27, 2017

@phimage well done. It is in 🎉, thanks a lot. It will be in version 4. @tbaranes thanks for your feedback.

@JakeLin JakeLin added this to the 4.0 milestone Mar 28, 2017
@phimage phimage deleted the feature-slider branch March 31, 2017 05:26
@tbaranes tbaranes modified the milestones: 4.x, 4.0 Apr 20, 2017
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.

None yet

4 participants