-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Update AlertState to use new APIs #794
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
Conversation
| case cancel(label: TextState?) | ||
| case `default`(label: TextState) | ||
| case destructive(label: TextState) | ||
| public enum ButtonRole { |
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.
This is breaking...but the rename seemed worth it to unify with Apple's APIs...
| withTransaction(self.transaction) { | ||
| self.wrappedValue = nil | ||
| } |
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.
withTransaction might not be needed here because self.transaction should already be applied to self.wrappedValue. It might even override other withTransaction/withAnimation calls higher up.
In the event you wanted to add support for .transaction()/.animation() to this binding, I've found the best way to maintain the animation precedence would be
extension Binding {
func isPresent<Wrapped>() -> Binding<Bool> where Value == Wrapped? {
.init(
get: { self.wrappedValue != nil },
set: { isPresent, transaction in
guard !isPresent else { return }
self.transaction(transaction).wrappedValue = nil
}
)
}
}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.
Interesting! Good to know, thanks @iampatbrown!
Assuming the new APIs are locked-in, this updates our existing APIs to consume the new ones and not the deprecated old ones. In the process I introduced the ability to do 3+ buttons on alerts.
Some food for thought:
ActionSheetStatebe renamed toConfirmationDialogState? Pretty wordy...AlertStateand we could introduce something like: