Skip to content

Support generic label and rounded rectangle shape#2

Merged
mikakruschel merged 11 commits into
no-comment:mainfrom
bjorninge-no-personal:loop-improvements
Oct 17, 2023
Merged

Support generic label and rounded rectangle shape#2
mikakruschel merged 11 commits into
no-comment:mainfrom
bjorninge-no-personal:loop-improvements

Conversation

@dabear
Copy link
Copy Markdown
Contributor

@dabear dabear commented Oct 17, 2023

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.

var actionText : some View {
    Text("Slide to Insert Cannula")
        .accessibility(identifier: "button_next_action")
        .accessibility(label: Text("Slide to insert cannula identifier here"))
        //.actionButtonStyle(.primary)
}

struct ContentView: View {
    private func sliderCallback() async {
        try? await Task.sleep(for: .seconds(2))
    }
    var body: some View {
        VStack {
            GenericSlideButton(actionText, styling: .init(indicatorShape: .rectangular(cornerRadius: 10), indicatorBrightness: -0.3, backgroundColor: .blue), callback: sliderCallback)
        }
        .padding()
    }
}

#Preview {
    ContentView()
}
testslider3_—_ContentView_swift_—_Edited

( I will add this repo as a dependency to LoopKit/OmniBLE#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.

@dabear
Copy link
Copy Markdown
Contributor Author

dabear commented Oct 17, 2023

what do you think @mikakruschel @cameronshemilt ?

@dabear
Copy link
Copy Markdown
Contributor Author

dabear commented Oct 17, 2023

Defaults unchanged:

testslider3_—_SlideButton_swift

@cameronshemilt
Copy link
Copy Markdown
Member

cameronshemilt commented Oct 17, 2023

Hey @dabear,
thank you for the pull request! I had a look at your changes and have a few thoughts:

  1. 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) })
    }
}
  1. 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?

@dabear
Copy link
Copy Markdown
Contributor Author

dabear commented Oct 17, 2023

  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

@dabear
Copy link
Copy Markdown
Contributor Author

dabear commented Oct 17, 2023

I think your comments should be addressed now by the latest commit 0ce2f23

@mikakruschel
Copy link
Copy Markdown
Member

mikakruschel commented Oct 17, 2023

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.

@mikakruschel
Copy link
Copy Markdown
Member

And since you mentioned accessibility, I think adding an accessibilityRepresentation with just a Button might be a nice addition

Fixed formatting and renamed callback parameter
Comment thread Sources/SlideButton/SlideButton.swift Outdated
case .circular:
Circle()
case .rectangular(let cornerRadius):
RoundedRectangle(cornerRadius: cornerRadius ?? 0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be RoundedRectangle(cornerRadius: max(0, (cornerRadius ?? 0) - styling.indicatorSpacing)) for an even spacing between the indicator and background

@dabear
Copy link
Copy Markdown
Contributor Author

dabear commented Oct 17, 2023

ok, updated. It would be nice if this can be merged now :) I can accessibility after this is merged, in a separate PR

Comment thread Sources/SlideButton/SlideButton.swift Outdated
case .circular:
Capsule()
case .rectangular(let cornerRadius):
RoundedRectangle(cornerRadius: max(0, (cornerRadius ?? 0) - styling.indicatorSpacing))
Copy link
Copy Markdown
Member

@mikakruschel mikakruschel Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the indicatorShape needed the spacing subtracted. The mask (which masks the background) should still be using RoundedRectangle(cornerRadius: cornerRadius ?? 0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, updated as well.

@mikakruschel
Copy link
Copy Markdown
Member

Just so you know what the issue currently is, this is the current SlideButton
image
and this is what it should be:
image

@mikakruschel mikakruschel changed the title Accessibility improvements and support different shapes Support generic label and rounded rectangle shape Oct 17, 2023
@mikakruschel mikakruschel merged commit db1d6cc into no-comment:main Oct 17, 2023
@mikakruschel
Copy link
Copy Markdown
Member

Thanks for the contribution, @dabear!

@dabear dabear deleted the loop-improvements branch October 18, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants