-
-
Notifications
You must be signed in to change notification settings - Fork 40
Add ButtonRepresentable/TextRepresentable protocols for rendering Alert/Confirmation Dialog/Menu content #160
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
|
Awesome! Will take a look when I get through my current task |
aabewhite
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.
Couple of comments, but overall looks great
| #if SKIP | ||
| content = Button(bridgedRole: nil, action: { self.openURL(destination) }, bridgedLabel: bridgedLabel) | ||
| self.action = { self.openURL(destination) } | ||
| self.label = ComposeBuilder(view: bridgedLabel) |
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.
It isn't intuitive, but should use ComposeBuilder.from { bridgedLabel }. The "view:" constructor is for when we know the view is something other than another ComposeBuilder
| public final class ShareLink : View { | ||
| public final class ShareLink : View, ButtonRepresentable { | ||
| private static let defaultSystemImageName = "square.and.arrow.up" | ||
| private static let defaultTitle = "Share..." |
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.
You should use the literal constructors or make this a LocalizedStringKey, otherwise there's no way to localize this string
Followup to #150 and #125
ButtonRepresentableprotocolTextRepresentableprotocolThis simplifies the code a bit and supports more cases for displaying these views in Alert, Confirmation Dialog, and Menu contexts. I've attached a demo view below for testing these changes.
Let me know if this approach looks good!
swift test