fix(Button): Make Button a UIControl BREAKING CHANGE: Button.State enum has changed to UIControl.State. #79
Conversation
jmpg93
changed the title
…ica-ios into apps/button-to-uicontrol
jmpg93
changed the title
…ica-ios into apps/button-to-uicontrol
…ica-ios into apps/button-to-uicontrol
…ica-ios into apps/button-to-uicontrol
|
|
||
| private lazy var animator = UIViewPropertyAnimator( | ||
| duration: Constants.animationDuration, | ||
| controlPoint1: Constants.animationCurveControlPoint1, | ||
| controlPoint2: Constants.animationCurveControlPoint2 | ||
| ) | ||
|
|
||
| private lazy var backingButton = BackingButton() |
There was a problem hiding this comment.
No need for backingButton anymore.
| override open var state: UIControl.State { | ||
| if isLoading { | ||
| return .loading | ||
| } else if !isEnabled { | ||
| return .disabled | ||
| } else if isSelected || isHighlighted { | ||
| return .selected | ||
| } else { | ||
| return .normal | ||
| } | ||
| } |
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) | ||
| } | ||
| } |
There was a problem hiding this comment.
We need to "disable" the button when it is loading.
| override func accessibilityActivate() -> Bool { | ||
| if state.shouldBackingButtonBeEnabled { | ||
| backingButton.sendActions(for: .touchUpInside) | ||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not needed. The UIContol handles all this for us, based on the state property.
| } else if previousState == .loading { | ||
| } else if isShowingLoadingAnimation { |
There was a problem hiding this comment.
I've added this isShowingLoadingAnimation to remove the concept of previousState.
| } | ||
|
|
||
| func objc_setDisabledState() { | ||
| state = .disabled | ||
| 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))
| override open var state: UIControl.State { | ||
| if isLoading { | ||
| return .loading | ||
| } else if !isEnabled { | ||
| return .disabled | ||
| } else if isSelected || isHighlighted { | ||
| return .selected | ||
| } else { | ||
| return .normal | ||
| } | ||
| } |
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...
jmpg93
dismissed stale reviews from jmbrocal, JulianAlonso, and pbartolome
via
e233172
| 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 isLoading property to false, that's it. If you set the isEnabled property to false while loading, the button will be disabled as soon as you set the isLoading property back to false. I do not see anything wrong here.
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!
# [9.0.0](v8.0.1...v9.0.0) (2021-02-17) ### Bug Fixes * **Button:** Make Button a UIControl BREAKING CHANGE: Button.State enum has changed to UIControl.State. ([#79](#79)) ([4b57222](4b57222)) * **ColorPalette:** fix backgroundPromo color of O2ClassicColorPalette ([#75](#75)) ([fd87da0](fd87da0)) * **DataCard:** fix border color of card ([cb4c3b0](cb4c3b0)) * **HighlightedCard:** add support for xib and storyboard ([5e3afa7](5e3afa7)) * **HighlightedCard:** fix showActionButton and showCloseButton so that the value of the variables makes sense in English ([b6a2921](b6a2921)) * **HighlightedCard:** nake init with parameters: `(title:subtitle:rightImage:actionButtonStyle:)` configures the correct styles. ([31f2bf6](31f2bf6)) * **HighlightedCard:** set multi lines support for both texts (title and subtitle) ([#77](#77)) ([657a3bc](657a3bc)) * **HighlightedCard,MediaCard,MediaCard,Stepper:** fix border color by replacing "divider" by "border" of the color palette and set the border size to 1 px ([66e4aae](66e4aae)) * **MediaCard:** fix border color of card and vertical spacing after headline ([f62b6f2](f62b6f2)) * **Stepper:** Update Lottie stroke width ([#76](#76)) ([0c9a35f](0c9a35f)) ### BREAKING CHANGES * **HighlightedCard:** texts (title and subtitle) of HighlightedCard have no restrictions of number of lines and both add support multi lines.
|
🎉 This PR is included in version 9.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Hi there!
This review is about making
Buttonto inherit fromUIControl. It will fix a few bugs we have right now regardingaddTargetmethod (which tried to simulateUIControlusage). It will also simplify the current solution.I have to change the current implementation to move from a single
stateproperty to a group of properties which computes the state of theButton. Meaning that we will move from thisbutton.state = .loadingto thisbutton.isLoading = true.Also, I've added new tests to check that the button stays the same when toggling those new properties:
isEnabled,isSelected, andisLoading.Enjoy.