Skip to content
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

Clarify wording for Learn More screen in Uncertain Delivery State #104

Merged
merged 4 commits into from
Jan 26, 2024

Conversation

marionbarker
Copy link
Collaborator

@marionbarker marionbarker commented Oct 30, 2023

Here's a first cut for modifying the Learn More screen for DASH pods.

The top part of the graphic is what is displayed now.
The bottom part matches the current commit.

For both cases, I had to scroll up to read all the text.

Removed graphic because I updated words based on comments. See later comment for new graphics.

@marionbarker
Copy link
Collaborator Author

marionbarker commented Nov 1, 2023

Thanks for the input.

Edited to remove graphics on 25 January 2024.
Changes words for consistency with LoopKit/OmniKit#23

@motinis
Copy link

motinis commented Nov 1, 2023

Looks great - the PR on the slider needs to include this Deactivate Pod button too

@culdeus
Copy link

culdeus commented Nov 13, 2023

Can I ask why there needs to be this deactivate pod button here at all. If the pod has lost comm, how do you deactivate it from this menu?

@ps2 ps2 changed the title Clarify wording for Learn More screen when Unable to Reach Pod Clarify wording for Learn More screen in Uncertain Delivery State Nov 13, 2023
@ps2
Copy link

ps2 commented Nov 13, 2023

Yes, it should skip the "deactivate" part, and just have a "Forget Pod" button. There does need to be a way to get out of this state, if comms cannot eventually be restored.

@marionbarker
Copy link
Collaborator Author

marionbarker commented Nov 14, 2023

Making changes one step at a time.

This commit

  • adds a primary button with the label "Keep Waiting", which dismisses this screen
  • renames the destructive button from "Deactivate Pod" to "Discard Pod"
  • no logic change was made to the action taken by that destructive button

Edited to remove graphics on 25 January 2024.
Changed words for consistency with LoopKit/OmniKit#23

@marionbarker
Copy link
Collaborator Author

I will need guidance if additional modifications are desired to the actions. Right now, the next screen after tapping Discard Pod (on the screen in prior comment) is the Deactivate Pod screen, which, if comms still fail, takes you to the Discard Pod screen as shown in the graphic below.

pr-104-unmodified-deactivate-then-discard-screen

Copy link

@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.

LGTM! Thanks!

@marionbarker
Copy link
Collaborator Author

See comment when testing similar change for OmniKit: LoopKit/OmniKit#23 (comment)

@marionbarker
Copy link
Collaborator Author

Updated with the graphics that match the latest commit (25 January 2024). After working with the uncertain comms for OmniKit, I modified those words then came back to this PR and modified them for consistency.

Top row - comes from LoopKit and is not modified.
Middle row - shows top and bottom of the screen addressed by this PR
Bottom row - shows the screen shown after comms are restored (not modified by this PR)

omnible-pr-104

Button(action: {
self.model.podDeactivationChosen()
}) {
Text(LocalizedString("Deactivate Pod", comment: "Button title to deactive pod on uncertain program"))
Text(LocalizedString("Discard Pod", comment: "Button title to discard pod on uncertain program"))
Copy link

Choose a reason for hiding this comment

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

I think this still takes the user to deactivate, rather than discard, but I'm on the fence about which is better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it does because I couldn’t figure out how to change it.

But on the other hand, if it succeeds in reaching the pod, the pod gets deactivated so there’s no screamer but at the same time you’re mad because you gave up too soon.

@ps2 ps2 merged commit 976a7d4 into LoopKit:dev Jan 26, 2024
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