Closed dabear closed 11 months ago
what do you think @mikakruschel @cameronshemilt ?
Defaults unchanged:
Hey @dabear, thank you for the pull request! I had a look at your changes and have a few thoughts:
Why split the slide button into two differently named views? I think additional initialisers that keep backwards compatibility would work better:
extension SlideButton where Label == Text {
public init(_ titleKey: LocalizedStringKey, styling: Styling = .default, callback: @escaping () async -> Void) {
self.init(styling: styling, callback: callback, label: { Text(titleKey) })
}
public init<S>(_ title: S, styling: Styling = .default, callback: @escaping () async -> Void) where S: StringProtocol {
self.init(styling: styling, callback: callback, label: { Text(title) })
}
}
As you may have seen above, I would also tweak the new View based initialiser to make it similar to the standard SwiftUI Button:
public init(styling: Styling = .default, callback: @escaping () async -> Void, @ViewBuilder label: () -> Label) {
self.title = label()
self.callback = callback
self.styling = styling
self._offset = .init(initialValue: styling.indicatorSpacing)
}
Note: You will have to also remove the @ViewBuilder
in line 40.
It otherwise looks fine to me. What do you think @mikakruschel?
1) The split was mainly to preserve backward compatibility (i.e. the new version is a drop in replacement for the previous version). If this is of no concern (it's up to you really) then the SlideButton can be generic itself. I've tested this as well, but if we do go for this solution, the Styling struct can no longer be an extension of SlideButton, since static properties on generics is not allowed by the compiler.
public extension SlideButton {
/// A struct that defines the styling options for a `SlideButton`.
struct Styling {
public static let `default`: Self = .init() //not allowed if Slidebutton is generic
I think your comments should be addressed now by the latest commit 0ce2f23
Hey @dabear, thanks for your contribution! I opened a PR on your fork to fix some code formatting issues and I renamed the callback
parameter to action
to be more consistent with the Button initializer.
And since you mentioned accessibility, I think adding an accessibilityRepresentation
with just a Button might be a nice addition
ok, updated. It would be nice if this can be merged now :) I can accessibility after this is merged, in a separate PR
Just so you know what the issue currently is, this is the current SlideButton
and this is what it should be:
Thanks for the contribution, @dabear!
This introduces a generic version of the slidebutton which can support views as title instead of String. This is necessary if you want to pass inn some Modified Text or other views as title.
( I will add this repo as a dependency to https://github.com/LoopKit/OmniBLE/pull/99 if this gets accepted. )
It also adds support for shapes other than circles, and in cases you want a solid background, you can choose to set the indicator's background relative to the overall background by setting indicatorBrightness to a value between -1 and +1.
Note that, to maintain backward compatibility there are now both a SlideButton and a GenericSlideButton. This allows the caller to use SlideButton as before without any changes. In that case there will be no changes for the enduser.
All the new options are by default turned off.