fix(Button): Make Button a UIControl BREAKING CHANGE: Button.State enum has changed to UIControl.State. #79
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
|
|
||
| import UIKit | ||
|
|
||
| open class Button: UIControl { | ||
| private enum Constants { | ||
| static let animationDuration: TimeInterval = 0.3 | ||
| static let animationCurveControlPoint1 = CGPoint(x: 0.77, y: 0) | ||
|
|
@@ -17,14 +17,6 @@ open class Button: UIView { | |
| static let borderWidth: CGFloat = 1.5 | ||
| } | ||
|
|
||
| public struct Style { | ||
| public let allowsBleedingAlignment: Bool | ||
| public let stateStyleByState: [State: StateStyle] | ||
|
|
@@ -43,7 +35,7 @@ open class Button: UIView { | |
| } | ||
|
|
||
| public init(allowsBleedingAlignment: Bool, | ||
| stateStyleByState: [State: Button.StateStyle], | ||
| overriddenSizes: Button.Style.OverriddenSizes? = nil) { | ||
| self.allowsBleedingAlignment = allowsBleedingAlignment | ||
| self.stateStyleByState = stateStyleByState | ||
|
|
@@ -85,21 +77,15 @@ open class Button: UIView { | |
| set { container.loadingTitle = newValue } | ||
| } | ||
|
|
||
| private var overridenAccessibilityLabel: String? | ||
| private var isShowingLoadingAnimation = false | ||
|
|
||
| private lazy var animator = UIViewPropertyAnimator( | ||
| duration: Constants.animationDuration, | ||
| controlPoint1: Constants.animationCurveControlPoint1, | ||
| controlPoint2: Constants.animationCurveControlPoint2 | ||
| ) | ||
|
|
||
| private lazy var container = ButtonContentView() | ||
|
|
||
| public convenience init() { | ||
|
|
@@ -114,7 +100,6 @@ open class Button: UIView { | |
|
|
||
| self.title = title | ||
| self.loadingTitle = loadingTitle | ||
| commonInit() | ||
| } | ||
|
|
||
|
|
@@ -139,24 +124,56 @@ open class Button: UIView { | |
| UIEdgeInsets(top: 0, left: leftBleedingInsets, bottom: 0, right: rightBleedingInsets) | ||
| } | ||
|
|
||
| override public var intrinsicContentSize: CGSize { | ||
| container.intrinsicContentSize | ||
| } | ||
|
|
||
| public var isLoading = false { | ||
| didSet { | ||
| didUpdateState() | ||
| } | ||
| } | ||
|
|
||
| override open var isHighlighted: Bool { | ||
| didSet { | ||
| didUpdateState() | ||
| } | ||
| } | ||
|
|
||
| override open var isSelected: Bool { | ||
| didSet { | ||
| didUpdateState() | ||
| } | ||
| } | ||
|
|
||
| override open var isEnabled: Bool { | ||
| didSet { | ||
| didUpdateState() | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. maybe we should modify the isLoading flag when setting one of these to false/true, E.g. isEnabled=true, isLoading=true -> we set isEnabled=false -> the button won't change. In this scenario, would it make sense to se set isLoading=false? There was a problem hiding this comment. I prefer that changing the value of one property does not affect the others. Enable and disable are different things from loading and not loading. If the button is loading and you want to stop loading, you must set the However, I dot no have a strong opinion here. I will do you what you guys think is better. There was a problem hiding this comment. make sense, it was just something that I found weird, but it can work as you said, thanks! |
||
|
|
||
| override open var state: UIControl.State { | ||
| if isLoading { | ||
| return .loading | ||
| } else if !isEnabled { | ||
| return .disabled | ||
| } else if isSelected || isHighlighted { | ||
| return .selected | ||
| } else { | ||
| return .normal | ||
| } | ||
| } | ||
|
Comment on lines
+155
to
+165
There was a problem hiding this comment. State is now a computed property. There was a problem hiding this comment. the only thing that bothers me is being able to set invalid combinations, like "isLoading=true" and "isEnabled=true"... or "isSelected=true" and "isEnabled=false", our component doesn't support it so it is what it is... |
||
|
|
||
| override open func hitTest(_ point: CGPoint, with event: UIEvent?) -> UIView? { | ||
| guard !isLoading else { return nil } | ||
| return super.hitTest(point, with: event) | ||
| } | ||
| } | ||
|
Comment on lines
+167
to
+171
There was a problem hiding this comment. We need to "disable" the button when it is loading. |
||
|
|
||
| @objc public extension Button { | ||
| override var accessibilityLabel: String? { | ||
| get { | ||
| if isLoading { | ||
| return loadingTitle | ||
| } else if overridenAccessibilityLabel != nil { | ||
| return overridenAccessibilityLabel | ||
|
|
@@ -168,15 +185,6 @@ open class Button: UIView { | |
| overridenAccessibilityLabel = newValue | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
-172
to
188
There was a problem hiding this comment. Not needed. The |
||
|
|
||
| private extension Button { | ||
|
|
@@ -217,7 +225,6 @@ private extension Button { | |
| func commonInit() { | ||
| setUpView() | ||
| setUpContainer() | ||
| updateStyle() | ||
| } | ||
|
|
||
|
|
@@ -228,33 +235,38 @@ private extension Button { | |
| } | ||
| layer.borderWidth = Constants.borderWidth | ||
| isAccessibilityElement = true | ||
| updateTraits() | ||
| } | ||
|
|
||
| func setUpContainer() { | ||
| container.isUserInteractionEnabled = false | ||
| addSubview(withDefaultConstraints: container) | ||
| } | ||
|
|
||
| func applyStyleColors() { | ||
| let stateStyle: StateStyle? | ||
|
|
||
| if isLoading { | ||
| stateStyle = style.stateStyleByState[.loading] | ||
| } else if !isEnabled { | ||
| stateStyle = style.stateStyleByState[.disabled] | ||
| } else if isSelected || isHighlighted { | ||
| stateStyle = style.stateStyleByState[.selected] | ||
| } else { | ||
| stateStyle = style.stateStyleByState[.normal] | ||
| } | ||
|
|
||
| guard stateStyle != nil else { | ||
| preconditionFailure("Style \(style) does not have stateStyle for state \(state). Check that the current style is defined properly.") | ||
| } | ||
|
|
||
| container.textColor = stateStyle!.textColor | ||
| backgroundColor = stateStyle!.backgroundColor | ||
| layer.borderColor = stateStyle!.borderColor.cgColor | ||
| } | ||
|
|
||
| func didUpdateState() { | ||
| if isLoading { | ||
| animator.stopAnimation(true) | ||
|
|
||
| // transition to loading | ||
|
|
@@ -265,9 +277,10 @@ private extension Button { | |
| self?.container.transitionToLoading() | ||
| self?.applyStyleColors() | ||
| } | ||
| updateTraits() | ||
| isShowingLoadingAnimation = true | ||
| animator.startAnimation() | ||
| } else if isShowingLoadingAnimation { | ||
|
Comment on lines
-270
to
+283
There was a problem hiding this comment. I've added this |
||
| animator.stopAnimation(true) | ||
|
|
||
| // transition to normal | ||
|
|
@@ -278,84 +291,57 @@ private extension Button { | |
| animator.addCompletion { [weak self] _ in | ||
| guard let s = self else { return } | ||
| UIAccessibility.post(notification: .layoutChanged, argument: s.accessibilityLabel) | ||
| s.updateTraits() | ||
| } | ||
| isShowingLoadingAnimation = false | ||
| animator.startAnimation() | ||
| } else { | ||
| applyStyleColors() | ||
| updateTraits() | ||
| } | ||
| } | ||
|
|
||
| func updateTraits() { | ||
| accessibilityTraits = state.accesibilityTraits | ||
| } | ||
| } | ||
|
|
||
| private extension Button.State { | ||
| var accesibilityTraits: UIAccessibilityTraits { | ||
| if contains(.disabled) || contains(.loading) { | ||
| return [.button, .notEnabled] | ||
| } else { | ||
| return .button | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // MARK: Objective-C API | ||
|
|
||
| @objc public extension Button { | ||
| func objc_setNormalState() { | ||
| isLoading = false | ||
| isSelected = false | ||
| isEnabled = true | ||
| } | ||
|
|
||
| func objc_setLoadingState() { | ||
| isLoading = true | ||
| } | ||
|
|
||
| func objc_setDisabledState() { | ||
| isEnabled = false | ||
|
There was a problem hiding this comment. should this also change "isLoading" to false? There was a problem hiding this comment. I prefer not (same reason as here #79 (comment)) |
||
| } | ||
|
|
||
| func objc_setLinkStyle() { | ||
| style = .link | ||
| } | ||
| } | ||
|
|
||
| // MARK: Useful extensions | ||
|
|
||
| public extension Button.State { | ||
| static let loading = UIControl.State(rawValue: 1 << 50) // Arbitrary value | ||
| } | ||
|
|
||
| extension Button.State: Hashable {} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for backingButton anymore.