-
Notifications
You must be signed in to change notification settings - Fork 32
Added support for alert without title #306
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
base: main
Are you sure you want to change the base?
Conversation
marcprux
left a comment
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 overall, and thanks for the screenshots! Just one question about the change to an optional init Text parameter…
| } | ||
|
|
||
| public func alert(_ title: Text, isPresented: Binding<Bool>, @ViewBuilder actions: () -> any View) -> any View { | ||
| public func alert(_ title: Text?, isPresented: Binding<Bool>, @ViewBuilder actions: () -> any View) -> any View { |
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.
Why the change to an optional Text parameter? That doesn't match the View.alert on Darwin, where it isn't optional, so I assume it wouldn't compile for iOS if you pass a nil argument (unless I'm missing another extension somewhere)?
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.
Thanks for pointing that out!
You’re right, View.alert on Darwin does indeed not accept an optional Text. I initially made the parameter optional so I could forward the “empty title” case by passing nil from the other overloads.
However, after revisiting this, I realized that this isn’t necessary as I can derive the underlying string from the Text via localizedTextString() and then handle the empty title case directly when calling the AlertPresentation.
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.
Great. I'll give this a trial run tonight (or maybe tomorrow) and then approve it.
I assume there aren't any corresponding https://github.com/skiptools/skip-fuse-ui/blob/main/Sources/SkipSwiftUI/Text/Text.swift changes that need to be made, since we aren't adding or removing any public API?
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.
Perfect, thanks! 😊
Yes, as far as I’m aware, no corresponding changes in SkipFuse are needed, as the adjustments are limited to the internal SkipUI implementation and don’t affect the public API.
This PR changes the
alertview modifier to match the iOS behavior if no / an empty title is used.Example SwiftUI code to test the changes:
iOS:

Android (Before):

Android (After):

Thank you for contributing to the Skip project! Please use this space to describe your change and add any labels (bug, enhancement, documentation, etc.) to help categorize your contribution.
Skip Pull Request Checklist:
swift test