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

Update PaddingDesignable to set rects for text, edit, and placeholder instead of UIView spacer #492

Merged
merged 6 commits into from Jun 9, 2017

Conversation

Projects
None yet
4 participants
@SD10
Member

SD10 commented Jun 2, 2017

Summary of Pull Request:

This pull request changes the implementation of PaddingDesignable to set the padding by insetting the bounds of the text, editing, and placeholder rects -- as opposed to the previous approach that used a UIView as a spacer.

To Do:

  • CHANGELOG
@IBAnimatableBot

This comment has been minimized.

Show comment
Hide comment
@IBAnimatableBot

IBAnimatableBot Jun 2, 2017

2 Warnings
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
⚠️ Consider adding / updating the demo ap.

Generated by 🚫 Danger

IBAnimatableBot commented Jun 2, 2017

2 Warnings
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
⚠️ Consider adding / updating the demo ap.

Generated by 🚫 Danger

@JakeLin

We discussed this before, your approach will have better control. I give 👍 for this PR. There are some breaking changes,can you update the CHANGELOG. let's discuss how can we ship it.

// MARK: - Helper Enum
fileprivate enum Edge {
case left(CGFloat)

This comment has been minimized.

@JakeLin

JakeLin Jun 2, 2017

Member

I like this idea, can we use it in our real API for the library? But don't know how to support IB yet.

@JakeLin

JakeLin Jun 2, 2017

Member

I like this idea, can we use it in our real API for the library? But don't know how to support IB yet.

This comment has been minimized.

@SD10

SD10 Jun 2, 2017

Member

I have to see if this can be used instead of the helper methods in PaddingDesignable.
Less code repetition.

@SD10

SD10 Jun 2, 2017

Member

I have to see if this can be used instead of the helper methods in PaddingDesignable.
Less code repetition.

This comment has been minimized.

@SD10

SD10 Jun 8, 2017

Member

Couldn't manage to reuse this code internally, just helps clean up testing

@SD10

SD10 Jun 8, 2017

Member

Couldn't manage to reuse this code internally, just helps clean up testing

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes Jun 2, 2017

Member

Looks good beside the missing points your reported :)

Member

tbaranes commented Jun 2, 2017

Looks good beside the missing points your reported :)

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin Jun 3, 2017

Member

Because there are breaking changes, I came up an idea, Should we keep the old variables, for example, paddingLeft will apply to all leftTextPadding, leftEditPadding and leftPlaceholderPadding . If those will have more priority, if we set leftTextPadding and paddingLeft. We use leftTextPadding over paddingLeft. I know the names are not consistent, it should be leftPadding instead of paddingLeft. But I think we need to fix it for next big release. Can I have you guys opinions on this? @SD10 @tbaranes thanks

Member

JakeLin commented Jun 3, 2017

Because there are breaking changes, I came up an idea, Should we keep the old variables, for example, paddingLeft will apply to all leftTextPadding, leftEditPadding and leftPlaceholderPadding . If those will have more priority, if we set leftTextPadding and paddingLeft. We use leftTextPadding over paddingLeft. I know the names are not consistent, it should be leftPadding instead of paddingLeft. But I think we need to fix it for next big release. Can I have you guys opinions on this? @SD10 @tbaranes thanks

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Jun 3, 2017

Member

I don't think we should be too concerned about API breaking changes if they enhance they project.
I hope I can add a lot more value to IBAnimatable in the long run so breaking changes will be inevitable.

That being said, I do think heavily documenting these changes and providing releases accordingly is extremely important.

One approach I think we could take is to do something like this:
We can use an @available attribute to trigger a deprecation warning on the old API
Then call the new implementation from within that API.

For example (just a rough idea):

public protocol PaddingDesignable: class {

  @available(*, deprecated: 4.2.0, message: "Deprecated. Please use leftTextPadding, leftEditPadding, leftPlaceholderPadding.")
  var paddingLeft: CGFloat { get set }
  
  var leftTextPadding: CGFloat { get set }
  var leftEditPadding: CGFloat { get set }
  var leftPlaceholderPadding: CGFloat { get set }
  
}

extension PaddingDesignable {

  var paddingLeft: CGFloat {
    get {
      return max(leftTextPadding, leftEditPadding, leftPlaceholderPadding)
    }
    set {
      leftTextPadding = newValue
      leftEditPadding = newValue
      leftPlaceholderPadding = newValue
    }
  }
}

We could even add a warning to the message saying that in IBAnimatable 5.0 this property will be dropped completely. It's like a way of phasing out the old API.

Member

SD10 commented Jun 3, 2017

I don't think we should be too concerned about API breaking changes if they enhance they project.
I hope I can add a lot more value to IBAnimatable in the long run so breaking changes will be inevitable.

That being said, I do think heavily documenting these changes and providing releases accordingly is extremely important.

One approach I think we could take is to do something like this:
We can use an @available attribute to trigger a deprecation warning on the old API
Then call the new implementation from within that API.

For example (just a rough idea):

public protocol PaddingDesignable: class {

  @available(*, deprecated: 4.2.0, message: "Deprecated. Please use leftTextPadding, leftEditPadding, leftPlaceholderPadding.")
  var paddingLeft: CGFloat { get set }
  
  var leftTextPadding: CGFloat { get set }
  var leftEditPadding: CGFloat { get set }
  var leftPlaceholderPadding: CGFloat { get set }
  
}

extension PaddingDesignable {

  var paddingLeft: CGFloat {
    get {
      return max(leftTextPadding, leftEditPadding, leftPlaceholderPadding)
    }
    set {
      leftTextPadding = newValue
      leftEditPadding = newValue
      leftPlaceholderPadding = newValue
    }
  }
}

We could even add a warning to the message saying that in IBAnimatable 5.0 this property will be dropped completely. It's like a way of phasing out the old API.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Jun 3, 2017

Member

Another concern I have regarding keeping the old API is then we would have a total of 12 @IBInspectable properties just for the padding of a UITextField.

I already felt unease about having 9 with the new API and considered removing the side properties :(

Member

SD10 commented Jun 3, 2017

Another concern I have regarding keeping the old API is then we would have a total of 12 @IBInspectable properties just for the padding of a UITextField.

I already felt unease about having 9 with the new API and considered removing the side properties :(

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin Jun 3, 2017

Member
Member

JakeLin commented Jun 3, 2017

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Jun 3, 2017

Member

I agree. I can't think of a use case where I'd want to have different padding for the text rect, edit rect, and placeholder rect. It's usually the same on all 3.

So just to make sure we're on the same page, let's stick to just 3 properties but we'll configure the bounds internally because it will be more performant than having the overhead of a UIView.

Good thinking to keep it simple.

Member

SD10 commented Jun 3, 2017

I agree. I can't think of a use case where I'd want to have different padding for the text rect, edit rect, and placeholder rect. It's usually the same on all 3.

So just to make sure we're on the same page, let's stick to just 3 properties but we'll configure the bounds internally because it will be more performant than having the overhead of a UIView.

Good thinking to keep it simple.

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin Jun 4, 2017

Member

Cool, can we go ahead with only three properties first, add more if needed in the future?

Member

JakeLin commented Jun 4, 2017

Cool, can we go ahead with only three properties first, add more if needed in the future?

@SD10 SD10 changed the title from Allow users to set text, edit, and placeholder rects in PaddingDesignable to Update PaddingDesignable to set rects for text, edit, and placeholder instead of UIView spacer Jun 8, 2017

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Jun 8, 2017

Member

@JakeLin
alright I kept the same API and updated the implementation, so no changes to Documentation.
Just need to add CHANGELOG entry... and get a build to pass 😅
Let me know what you think.

Member

SD10 commented Jun 8, 2017

@JakeLin
alright I kept the same API and updated the implementation, so no changes to Documentation.
Just need to add CHANGELOG entry... and get a build to pass 😅
Let me know what you think.

@JakeLin

JakeLin approved these changes Jun 9, 2017

👍 expect for a comment

if paddingSide.isNaN {
return
return insets(left: paddingLeft, right: paddingRight)

This comment has been minimized.

@JakeLin

JakeLin Jun 9, 2017

Member

How about one of this property is nan? We may need to check individually

@JakeLin

JakeLin Jun 9, 2017

Member

How about one of this property is nan? We may need to check individually

This comment has been minimized.

@SD10

SD10 Jun 9, 2017

Member

I did the check inside of insets(left:right:). If either isNaN it will default to 0.
I could extract the logic if it's not clear.

@SD10

SD10 Jun 9, 2017

Member

I did the check inside of insets(left:right:). If either isNaN it will default to 0.
I could extract the logic if it's not clear.

This comment has been minimized.

@JakeLin

JakeLin Jun 9, 2017

Member

Good👍

@JakeLin

JakeLin Jun 9, 2017

Member

Good👍

configurePaddingSide()
}
override open func placeholderRect(forBounds bounds: CGRect) -> CGRect {
return paddedRect(forBounds: bounds)
}

This comment has been minimized.

@JakeLin

JakeLin Jun 9, 2017

Member

👍 expect for a comment

@JakeLin

JakeLin Jun 9, 2017

Member

👍 expect for a comment

This comment has been minimized.

@SD10

SD10 Jun 9, 2017

Member

Except**? Meaning you would like me to add some comments? 😅 Sorry

@SD10

SD10 Jun 9, 2017

Member

Except**? Meaning you would like me to add some comments? 😅 Sorry

This comment has been minimized.

@JakeLin

JakeLin Jun 9, 2017

Member

No, I mean my comment😅 I think you are good to go.

@JakeLin

JakeLin Jun 9, 2017

Member

No, I mean my comment😅 I think you are good to go.

SD10 added a commit that referenced this pull request Jun 9, 2017

@SD10 SD10 merged commit 6aa3b03 into master Jun 9, 2017

2 of 4 checks passed

buddybuild: IBAnimatable (iOS, IBAnimatable) Build error
Details
buddybuild: IBAnimatable (iOS, IBAnimatableApp) Build error
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tbaranes tbaranes deleted the feature/textfield-padding-designable branch Jun 9, 2017

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