Add ButtonRepresentable/TextRepresentable protocols for rendering Alert/Confirmation Dialog/Menu content#160
Add ButtonRepresentable/TextRepresentable protocols for rendering Alert/Confirmation Dialog/Menu content#160ky-is wants to merge 5 commits intoskiptools:mainfrom
Conversation
|
Awesome! Will take a look when I get through my current task |
aabewhite
left a comment
There was a problem hiding this comment.
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.
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.
You should use the literal constructors or make this a LocalizedStringKey, otherwise there's no way to localize this string
|
Catching up on the backlog, I would merge this but it needs some conflict resolution to merge with recent changes. I'm going to put this into Draft state until such time as someone can spend time getting it back into mergable state. |
| public convenience init(item: String, subject: Text? = nil, message: Text? = nil) { | ||
| self.init(text: Text(item), subject: subject, message: message) { | ||
| Image(systemName: Self.defaultSystemImageName) | ||
| Label("Share...", systemImage: Self.defaultSystemImageName) |
There was a problem hiding this comment.
I think we might be able to use stringResource(android.R.string.share) from androidx.compose.ui.res.stringResource to get free localization here.
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