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

Adding the ability to pass in an image for the circle slider view. #4

Merged
merged 5 commits into from
Jan 4, 2017

Conversation

campierce88
Copy link
Contributor

Also adding the ability to specify which direction the slider will transition from.

Cameron Pierce added 2 commits December 5, 2016 15:38
Also adding the ability to specify which direction the slider will transition from.
@codecov-io
Copy link

codecov-io commented Dec 6, 2016

Current coverage is 72.80% (diff: 48.00%)

Merging #4 into master will decrease coverage by 14.43%

@@             master         #4   diff @@
==========================================
  Files             4          4          
  Lines            94        125    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits             82         91     +9   
- Misses           12         34    +22   
  Partials          0          0          

Powered by Codecov. Last update dafb50d...e7f04e3

@quver quver self-requested a review December 18, 2016 20:30
Copy link
Owner

@quver quver left a comment

Choose a reason for hiding this comment

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

Please check comments, looks very good but syntax has to be fixed

var borderWidth: CGFloat
var borderColor: CGColor

public init(borderWidth width: CGFloat, borderColor color: CGColor) {
Copy link
Owner

Choose a reason for hiding this comment

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

This init is not necessary

self.firstView = firstView
self.secondView = secondView
sliderCircle = SlidableImage.setupSliderCircle()
self.sliderCircle = SlidableImage.setupSliderCircle(sliderImage: sliderImage)
self.slideDirection = slideDirection ?? .right
Copy link
Owner

Choose a reason for hiding this comment

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

instead of this it would be clearer if you move it to init declaration slideDirection: SlideDirection? = .right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I move it to the init declaration then you can not call the init without a slideDirection param. Since this is an optional pram, it made more since to do the defaulting in the optional coalescing.

@@ -42,11 +63,11 @@ open class SlidableImage: UIView {
/// - frame: Frame size
/// - firstImage: First image for sliding
/// - secondImage: Second image for sliding
convenience public init(frame: CGRect, firstImage: UIImage, secondImage: UIImage) {
convenience public init(frame: CGRect, firstImage: UIImage, secondImage: UIImage, sliderImage: UIImage? = nil, slideDirection: SlideDirection? = nil) {
Copy link
Owner

Choose a reason for hiding this comment

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

Also, you should update comments

let mask = CAShapeLayer()
switch slideDirection {
case .left:
maskRectPath = UIBezierPath(rect: CGRect(x: maskLocation,
Copy link
Owner

Choose a reason for hiding this comment

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

Just CGRect declaration should be in case, it would be clearer what is really switching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure what you mean here. Can you elaborate?

mask.path = maskRectPath.cgPath
secondView.layer.mask = mask

sliderCircle.center.x = maskLocation
if slideDirection == .left || slideDirection == .right {
Copy link
Owner

Choose a reason for hiding this comment

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

this should be

switch slideDirection {
case .left, .right:
  sliderCircle.center.x = maskLocation
case .top, .bottom:
  sliderCircle.center.y = maskLocation
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

}

/// Private wrapper for setup view
fileprivate func initializeViews() {
clipsToBounds = true
sliderCircle.center = center
updateMask(location: center.x)

if slideDirection == .left || slideDirection == .right {
Copy link
Owner

Choose a reason for hiding this comment

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

same here

}

/// Private wrapper for setup circle slider view
///
/// - Returns: Slider circle
private class func setupSliderCircle() -> UIView {
private class func setupSliderCircle(sliderImage: UIImage? = nil) -> UIView {
Copy link
Owner

Choose a reason for hiding this comment

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

if you change method declaration you should update comments

@@ -8,6 +8,23 @@

import UIKit

public enum SlideDirection: Int {
Copy link
Owner

Choose a reason for hiding this comment

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

Enum should have comment

case bottom
}

public struct SlidableImageBorder {
Copy link
Owner

Choose a reason for hiding this comment

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

struct also

@@ -88,24 +135,28 @@ open class SlidableImage: UIView {
@objc private func gestureHandler(_ panGestureRecognizer: UIPanGestureRecognizer) {
let location = panGestureRecognizer.location(in: firstView)

guard (bounds.minX...bounds.maxX ~= location.x) else {
return
if slideDirection == .left || slideDirection == .right {
Copy link
Owner

Choose a reason for hiding this comment

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

same here

@quver
Copy link
Owner

quver commented Dec 18, 2016

Also, sorry for delay, something went wrong with slack notifications

@campierce88
Copy link
Contributor Author

Ive added the requested changes that I understood. No worries on the delay, we've all been there :).

@quver
Copy link
Owner

quver commented Jan 4, 2017

I resolved conflicts, thanks for contribution 👍

@quver quver merged commit 0316464 into quver:master Jan 4, 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.

3 participants