-
Notifications
You must be signed in to change notification settings - Fork 132
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
Animated assign subscriber #17
Conversation
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.
Looks great! Have a few quick notes here :)
Co-authored-by: Shai Mishali <[email protected]>
Co-authored-by: Shai Mishali <[email protected]>
Co-authored-by: Shai Mishali <[email protected]>
Co-authored-by: Shai Mishali <[email protected]>
Co-authored-by: Shai Mishali <[email protected]>
Thanks @freak4pc - merged and removed the guard too. |
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.
Sorry for the delay here, crazy few days. Noticed two more quick notes that I wanted to ask you about.
/// ``` | ||
public func assign<Root: UIView>(to keyPath: ReferenceWritableKeyPath<Root, Self.Output>, on object: Root, animation: AssignTransition) -> AnyCancellable { |
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.
object[keyPath: keyPath] = value | ||
animations() | ||
}, | ||
completion: completion) | ||
}) | ||
.assign(to: keyPath, on: object) |
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.
Another question that popped to my head,
You're already setting the value inside the animation block, and the assign will do so again.
Maybe it's worth swapping the assign with something like sink(receiveValue: { _ in })
. Just to active the chain? (same for below)
Hey @icanzilb - any chance you have a moment to answer above questions? Happy to help carry this over the merge line with you |
@freak4pc yikes, it seems I missed this one - looking at it this afternoon 👍🏼 |
@freak4pc 👆🏼 |
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.
Yikes, was sure I already merged this.
Sorry @icanzilb ! Looks good.
This PR adds a variation of
assign(to:on:)
with an extra parameter allowing the user to set an animation transition to play when emitting output values.E.g. if you have:
It makes the updates to the label animated like so: