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

Tweak Simple mode by adding required and current OS #192

Merged
merged 18 commits into from
Aug 3, 2021

Conversation

natewalck
Copy link
Member

@natewalck natewalck commented Jul 16, 2021

This does a few things:

  • Add Required and Current OS versions
  • Tweak weights/titles

Also I propose making Deferral count hidden by default in Simple mode, but with an option to enable it. The thought process is one can use Simple mode for the first N days of nudging and only switch to full mode if people are ignoring it.

Here is what it looks like with deferralCount:

Screen Shot 2021-07-16 at 11 02 21 AM

And without:
Screen Shot 2021-07-16 at 11 02 11 AM

It looks a lot cleaner without the deferral count, so the option to disable it would be very good to have (or even disable by default, enable by option, which is my preference).

@natewalck natewalck changed the title Tweak minimal mode by adding required and current OS Tweak Simple mode by adding required and current OS Jul 16, 2021
@erikng
Copy link
Member

erikng commented Jul 16, 2021

Can you switch to the non-debug xcworkspace and take screenshots of just simple mode?

My first inclination is to say it's busy, but I think it's because both UIs are rendering in your example.

@natewalck
Copy link
Member Author

natewalck commented Jul 16, 2021

Here is everything enabled:

Screen Shot 2021-07-16 at 12 21 27 PM

Here is without Deferred count:

Screen Shot 2021-07-16 at 12 22 34 PM

I think in Simple mode, it might even be worth hiding "Days remaining" by default like so:

Screen Shot 2021-07-16 at 12 23 45 PM

@erikng
Copy link
Member

erikng commented Jul 16, 2021

I don't use simple mode, so would like anyone who is to chime in on these changes as it does remove UX.

Nudge/Preferences/DefaultPreferencesNudge.swift Outdated Show resolved Hide resolved
if !hideDeferralCount() {
HStack {
Text("Deferred Count:".localized(desiredLanguage: getDesiredLanguage()))
.font(.title3)
Copy link
Member

Choose a reason for hiding this comment

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

why did you move to title3? That may be why I dislike the change

also see my comment on standard mode

.font(.title3)
Text(manager.current.description)
.foregroundColor(.secondary)
.font(.title3)
Copy link
Member

Choose a reason for hiding this comment

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

IMO should be title2 to stay consistent with the other textfields. Standard mode keeps the same font weight for all of these UI fields

Copy link
Member Author

Choose a reason for hiding this comment

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

In Simple mode, title2 looks a little big, but maybe it's fine?. Here is title2 vs title3 for all of them:

title2:

Screen Shot 2021-07-16 at 1 54 09 PM

title3:

Screen Shot 2021-07-16 at 1 53 51 PM

Nudge/UI/StandardMode/LeftSide.swift Outdated Show resolved Hide resolved
Nudge/Utilities/Utils.swift Outdated Show resolved Hide resolved
Nudge/Utilities/Preferences.swift Outdated Show resolved Hide resolved
Nudge/Utilities/Utils.swift Outdated Show resolved Hide resolved
@natewalck
Copy link
Member Author

This should be ready for review again.

@macbm
Copy link

macbm commented Jul 16, 2021

We only use Simple Mode in our environment, and we like the changes Nate is proposing.
It would be great to have the options to show/hide "Days remaining to update", "Require OS Version" & "Current OS Version" if possible for better control/customization.

@w0de
Copy link

w0de commented Jul 16, 2021

+1 to @macbm's comments

This is a good change for our use of simple mode. Presently we overload the "More Info" button to further inform users, but that isn't device-state specific. This strikes a good balance and will probably have a positive impact on user touchpoints. As it is now, we would use this.

I have an aesthetic nitpick:

  • Is a bit busy
  • the mismatch between bold/not bold for day/deferral values & OS version values contributes to this
  • three buttons each of their own style and alignment contributes to this

Would perhaps a popover view allow displaying further information in simple mode more elegantly? Perhaps an option for configuring More Info in lieu of setting an aboutUpdateURL - or with this URL configuring a button inside the popover instead.

@natewalck
Copy link
Member Author

Here is the default Simple UI as of the latest commit:
Screen Shot 2021-07-23 at 10 39 55 AM

@natewalck
Copy link
Member Author

We only use Simple Mode in our environment, and we like the changes Nate is proposing.
It would be great to have the options to show/hide "Days remaining to update", "Require OS Version" & "Current OS Version" if possible for better control/customization.

We discussed this in Slack a little and for now we are going to try to settle on a standard simple mode UI that has only actionable information for the end user. In this case, its the required version of macOS and how many days they have left to update. The other information is a kind of noise by itself and having all 4 makes Simple mode not so simple.

I'm proposing the current diff is the new default and then we can talk about making them a preference or something like that.

@natewalck
Copy link
Member Author

For bonus points, added the device info button to simple mode:
Screen Shot 2021-07-23 at 12 53 08 PM

@natewalck
Copy link
Member Author

Completely and utterly changed my mind here about showing required OS version. People have no clue what they are on or what is required. Maybe this can be added to the ? long term for inquisitive minds, but for 98% of everyone it is not useful information as we are only showing this screen if they are indeed out of date.

Here is the latest:
Screen Shot 2021-07-23 at 2 47 14 PM

PS: Aligning stuff sucks. The more info button keeps moving 🙄

@erikng
Copy link
Member

erikng commented Jul 23, 2021

I like this.

@natewalck
Copy link
Member Author

Need review on this.

@natewalck natewalck force-pushed the nate/show-required-ver-minimal branch from 25a638e to 7f6896e Compare July 28, 2021 16:34
@natewalck natewalck changed the base branch from main to development July 28, 2021 16:34
@natewalck
Copy link
Member Author

Alright, this is rebased to Development and I confirmed it ran:

Screen Shot 2021-07-28 at 12 34 16 PM

@natewalck natewalck force-pushed the nate/show-required-ver-minimal branch from 7f6896e to bf18b0d Compare July 28, 2021 19:19
@@ -365,7 +365,7 @@ extension UserExperience {
// MARK: - UserInterface
struct UserInterface: Codable {
var fallbackLanguage: String?
var forceFallbackLanguage, forceScreenShotIcon: Bool?
var forceFallbackLanguage, forceScreenShotIcon, showDeferralCount: Bool?
Copy link
Member

Choose a reason for hiding this comment

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

Nit but all of the keys are alphabetical before this change. Can you move it down to L370?

@natewalck
Copy link
Member Author

I think we are good now at least on Big Sur.

Screen Shot 2021-07-28 at 4 08 05 PM

Screen Shot 2021-07-28 at 4 08 14 PM

Nudge/UI/SimpleMode/SimpleMode.swift Outdated Show resolved Hide resolved
Nudge/UI/SimpleMode/SimpleMode.swift Show resolved Hide resolved
Nudge/UI/SimpleMode/SimpleMode.swift Show resolved Hide resolved
Nudge/UI/SimpleMode/SimpleMode.swift Show resolved Hide resolved
Nudge/UI/SimpleMode/SimpleMode.swift Show resolved Hide resolved
@natewalck
Copy link
Member Author

Alright, by taking the ? button out of the centered vstack, we can avoid the showDeferralCount being enabled/disabled impacting the vertical spacing on the ?. This allows us to tweak the location of the ? independently of the other VStacks.

@natewalck
Copy link
Member Author

I did just notice the button location changes when deferral count is enable or disabled -_- Guess I'll figure that out now.

Screen Shot 2021-07-29 at 11 21 23 AM

Screen Shot 2021-07-29 at 11 20 50 AM

@natewalck
Copy link
Member Author

This is really dumb, but it works :D Button doesn't move at all (I tried various other combinations of Spacer() and padding, but nothing worked quite right).

Screen Shot 2021-07-29 at 12 29 02 PM

Screen Shot 2021-07-29 at 12 28 51 PM

@natewalck natewalck requested a review from erikng July 29, 2021 16:33
@erikng erikng merged commit eb27ef2 into development Aug 3, 2021
@erikng erikng deleted the nate/show-required-ver-minimal branch August 3, 2021 16:06
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.

None yet

4 participants