Skip to content

Insert cannula view improvements for OmniBLE#99

Merged
ps2 merged 11 commits into
LoopKit:devfrom
dabear:InsertCannulaViewImprovements
Oct 28, 2023
Merged

Insert cannula view improvements for OmniBLE#99
ps2 merged 11 commits into
LoopKit:devfrom
dabear:InsertCannulaViewImprovements

Conversation

@dabear
Copy link
Copy Markdown

@dabear dabear commented Oct 12, 2023

I saw a comment about the "inserting cannula" button is too easy to press involuntarily, or before the enduser is actually ready. This tries to rectify that by forcing the enduser to "slide to insert cannula" thereby making it slighly harder to insert the cannula by mistake.

This adds a dependency on dabear/SlideButton (https://github.dev/dabear/SlideButton ) which is a fork of no-comment/SlideButton( https://github.com/no-comment/SlideButton ) to omnible.
An open question is if this should live as a core part of loopkitui instead.

LoopWorkspace_—_InsertCannulaView_swift LoopWorkspace_—_InsertCannulaView_swift

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 13, 2023

Thoughts, @ps2?

@ps2
Copy link
Copy Markdown

ps2 commented Oct 16, 2023

Yes, there is understandably a good amount of anxiety around this action for some, and welcome a chance to make it feel more controlled. Questions below:

Reliability: it's very important that the activation mechanism is very reliable; i.e. users don't have trouble activating it, and we need to consider users with less fine motor control skills. As long as the tap region is large, and the action doesn't disengage if the user goes out of bounds vertically too easily, I think this seems fine.

Accessibility: How does this work during voiceover use?

Changes from upstream: Can you explain why we're not just using the original SlideButton repo?

Design: button corners that match the radius of the slide control do a better job of indicating that the control "fits" in the slot, and can be slid.

@ps2
Copy link
Copy Markdown

ps2 commented Oct 16, 2023

And the instruction steps above should probably not say "Tap below".

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 17, 2023

Reliability seems to be no problem, as it doesn't disengage.

Accessibility: Any good strategy for testing this, or articles to describe the problem? I could always add an extra accessibility label if that helps.

Changes from upstream: I was not initially able to modify the repo to accept "some View" (or the concrete type of the actionText variable in omnible) in instead of String for the title without breaking backward compatibility. However, I might have found a way, so will try to upstream changes.

Looks: better?
testslider3_—_ContentView_swift

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 17, 2023

upstream PR here: no-comment/SlideButton#2 .

@ps2
Copy link
Copy Markdown

ps2 commented Oct 17, 2023

Heh, I meant adjusting the frame corner radius to match the circular control, not vice versa. :)

Having a squarish control in the squarish frame might look a little better than circular control in the squarish frame, but the way it looks in the upstream repo is what I was referring to; i.e. circular control in a frame with matching corners.

As for testing voiceover, this is a good resource: https://developer.apple.com/documentation/accessibility/supporting_voiceover_in_your_app/. It takes a little time to figure out how to use voiceover, but it's really good to be able to see how people with vision impairments use the app. I'm more worried about enacting the control, vs the labeling (though that is important too).

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 17, 2023

The default in omnible and loop it is to have a squarish button for swiftui 's standard button. If you really want the slider to be be circular I can ofc easily do that. That also means that the standard buttons immediately preceding this one have squarish design while the slide button has circular. I guess I was going for consistency there.

@ps2
Copy link
Copy Markdown

ps2 commented Oct 17, 2023

This is a very different control (slider vs button), and I think visual difference is ok/nice here. The mixing of shapes (square/circle) on the single control is definitely not great, and I'm not a fan of the square/square slider. I think the original/upstream one looks ok. Would be good to check it in dark mode, too.

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 17, 2023

I'll make the change to circular version, and change to the upstream version as well, once my changes to upstream is merged.

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 19, 2023

Changed to upstream slidebutton.

Please tell me which of these options are better, and I'll change

  1. Current version, inherits some style from loop with actionButtonStyle(.primary):
    Cursor_and_LoopWorkspace_—_InsertCannulaView_swiftLoopWorkspace_—_InsertCannulaView_swift LoopWorkspace_—_InsertCannulaView_swiftLoopWorkspace_—_InsertCannulaView_swift

  2. Default from upstream, without actionButtonStyle(.primary):
    Cursor_and_LoopWorkspace_—_InsertCannulaView_swift_—_EditedLoopWorkspace_—_InsertCannulaView_swift_—_EditedLoopWorkspace_—_InsertCannulaView_swift_—_EditedLoopWorkspace_—_InsertCannulaView_swift_—_Edited

  3. Slidebutton with .actionButtonStyle(.primary) and indicatorColor: .accentColor, indicatorBrightness: -0.2, backgroundColor: .accentColor
    LoopWorkspace_—_InsertCannulaView_swift_—_EditedLoopWorkspace_—_InsertCannulaView_swift_—_Edited

LoopWorkspace_—_InsertCannulaView_swift_—_Edited LoopWorkspace_—_InsertCannulaView_swift_—_Edited
  1. Like option 2 Default from upstream, without actionButtonStyle(.primary), but with font explicitly set to (.headline):
    LoopWorkspace_—_InsertCannulaView_swift_—_EditedLoopWorkspace_—_InsertCannulaView_swift_—_Edited
LoopWorkspace_—_InsertCannulaView_swift_—_Edited LoopWorkspace_—_InsertCannulaView_swift_—_Edited

I'm leaning towards 2 or4, which is the default from upstream slidebutton, but I feel 3 has better contrast. Thoughts?

Bjørn Inge Berg added 2 commits October 19, 2023 15:05
@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 20, 2023

I chose option 4, Like option 2 Default from upstream, without actionButtonStyle(.primary), but with font explicitly set to (.headline)

Additionally I worked with upstream SlideButton to make it work correctly with voiceover. I consider this ready for merge

@ps2
Copy link
Copy Markdown

ps2 commented Oct 20, 2023

Thanks! I think option 4 is the right choice.

Comment thread OmniBLE/PumpManagerUI/ViewModels/InsertCannulaViewModel.swift Outdated
Copy link
Copy Markdown

@ps2 ps2 left a comment

Choose a reason for hiding this comment

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

I think this looks good from a code standpoint, with just a small naming nit. Might wait for some more testing reports, but then I'll plan to merge this. Would you be able to get the same changes applied to OmniKit? We try to keep them in sync. Thanks again!

@marionbarker
Copy link
Copy Markdown
Collaborator

I installed this on a test phone. See bug report (item 3 below)

  1. I love this change to the UX.
  2. I would like to see this expanded to the Deactivate Pod button as well.
  3. There's a problem with the continue button displayed after the cannula insertion is completed. It does not have the correct format (although it does work).

pr-99-after-cannula-is-inserted

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 21, 2023

Thanks. I'll look into the continue button, should be a Quick fix. Pete; i'll create a pr for omnikit as well

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 21, 2023

Thanks for the feedback, can you retest @marionbarker ?

@dabear dabear changed the title Insert cannula view improvements Insert cannula view improvements for OmniBLE Oct 21, 2023
@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 21, 2023

opened identical PR but for the omnikit repo: LoopKit/OmniKit#13

@marionbarker
Copy link
Copy Markdown
Collaborator

I confirm the Continue button matches the expected format on the screen displayed when the Cannula is Inserted.

@marionbarker
Copy link
Copy Markdown
Collaborator

I posted on zulipchat some alternative sizes for the SlideButton to address Patriq's concern for people who have poor motor control:

I got several PM indicating they like the middle size of the three that were shown (indicatorSize of 60, 90 or 120).
The size modification requires a one-line change (of line 99 of OmniBLE/PumpManagerUI/Views/InsertCannulaView.swift) from

SlideButton(action: {

to

SlideButton(styling: .init(indicatorSize: 90), action: {

If @ps2 has an opinion, let us know.

@ps2
Copy link
Copy Markdown

ps2 commented Oct 23, 2023

It should be standard size, I think. If the user has increased font size via dynamic type, I think it's fine if the button expands based on larger type, but generally we should target standard size hit boxes.

@motinis
Copy link
Copy Markdown

motinis commented Oct 25, 2023

I installed this on a test phone. See bug report (item 3 below)

  1. I love this change to the UX.
  2. I would like to see this expanded to the Deactivate Pod button as well.

Completely agree - Deactivate Pod should be the same. Btw I have seen as well sometimes when there is a communications issue Loop will display a message that you can either wait until communications are re-established or deactivate the pod (which is a button I think - I've never pushed it). I think that from a UX perspective anywhere we click deactivate pod it should then take us to a "Deactivate Pod" screen with the slider.

@dabear
Copy link
Copy Markdown
Author

dabear commented Oct 25, 2023

Anything preventing this from being merged?

@ps2
Copy link
Copy Markdown

ps2 commented Oct 28, 2023

Anything preventing this from being merged?

No, I just needed to get some time to look at this, and review it in the context of making a decision about whether preceeding screens are still valid, and whether it would make sense to do this for deactivate as well.

I just tried it out, and it seems fine. I think the transition from round button to square as the UI transitioned into the "in-progress" state was a little unusual, but not a blocker, and it does need to eventually transition into a standard action button for the "continue" state.

I'll go ahead and merge this. Thanks!

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.

4 participants