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

Toggling Release Dismissal #57

Merged
merged 4 commits into from
Nov 12, 2019
Merged

Toggling Release Dismissal #57

merged 4 commits into from
Nov 12, 2019

Conversation

mmstick
Copy link
Member

@mmstick mmstick commented Nov 8, 2019

  • The dismissal frame is now inside of the upgrade option box
  • The dismissal button is now a check button, which can be toggled
  • The daemon has been updated to take a boolean as an input and output

@mmstick mmstick requested review from a team November 8, 2019 18:13
@leviport
Copy link
Member

leviport commented Nov 8, 2019

The checkbox label and description make it look like the word "Dismiss" is duplicated. I think we could put the description text in the label since it's pretty short. I tried to make the change, but I broke stuff and gave up 😬

dismiss

@mmstick
Copy link
Member Author

mmstick commented Nov 8, 2019

The description should be in the check button's label now.

@leviport
Copy link
Member

leviport commented Nov 8, 2019

This may enter the realm of "unreasonable" and I don't think it would block release, but I thought it would be worth pointing out.

I noticed if I check and uncheck the "dismiss" box repeatedly, it locks up gnome control center for a little bit. In the logs, I noticed every time the box is checked or unchecked, it looks for available releases. I think that might be what slows it down when the checkbox is spammed.

@mmstick
Copy link
Member Author

mmstick commented Nov 8, 2019

We could cache the release information in the daemon between requests, so it doesn't need to happen every time.

@leviport
Copy link
Member

leviport commented Nov 8, 2019

Sounds like a good idea to me! Probably a low-priority fix though. I can't imagine too many people will rock the casbah spam the checkbox.

@isantop
Copy link
Contributor

isantop commented Nov 8, 2019

We should also revise the description to avoid having a negative within the description. Otherwise you have to enable the checkmark to disable the notifications, which creates a lot of confusion about what the control actually does and what state it's in.

I know we (including myself) initially dismissed this idea, but seeing the mockup I think a flat button labeled "Dismiss Notifications" is actually a better way to go with this. I'm having a hard time seeing an actual, valid use case for disabling the notifications with the intention of showing them again at a later date; the user would instead simply click the upgrade button directly. My initial thought on that might be that someone might be working on a system, want to disable the notifications momentarily, and then re-enable them for the primary user, but this is not a normal use case nor is it something that an advanced user couldn't do by simply resetting the configuration for this toggle.

@maria-komarova
Copy link
Contributor

I think switch is our best option for now. It looks like most of the settings have switches, including the Notifications. I also have a difficult time imagining anyone wanting to switch the notifications back on. On the other hand, having two buttons with different function where one is an action and another is a toggle might be more confusing. In this case, we should give a visual priority to the "Download" button because we want the users to upgrade. Hence, better avoid using two buttons. If we include two buttons, they will need to have the same height and length. That would make them very close in priority. And since the switch is what currently used throughout let's just stick to it.
The wording can actually be very simple. "Notifications for Pop!_OS 19.10" is what we have just discussed.

@leviport
Copy link
Member

The new switch looks good, however it seems its state is currently reversed. Default state is as pictured (switch off) and the notifications are displayed. Activating the switch suppresses the notifications.
new-switch-reversed

@brs17
Copy link
Contributor

brs17 commented Nov 11, 2019

@mmstick let's align the switch with the Download button on the right side.

@isantop
Copy link
Contributor

isantop commented Nov 11, 2019

This is closer-to-final layout and verbiage for this improvement:

Screenshot from 2019-11-11 14-46-38

@mmstick mmstick force-pushed the dismisser branch 2 times, most recently from 0e25975 to 4ca1e5d Compare November 11, 2019 21:44
@mmstick mmstick changed the title improvement: Support toggling dismissal of a release Toggling Release Dismissal Nov 11, 2019
- The dismissal frame is now inside of the upgrade option box
- The dismissal button is now a check button, which can be toggled
- The daemon has been updated to take a boolean as an input and output
@mmstick mmstick force-pushed the dismisser branch 2 times, most recently from 9b8ee5b to d91204f Compare November 11, 2019 21:57
@leviport
Copy link
Member

For all of our reference, here is the current implementation of the dismisser.
most-current

@brs17 brs17 added this to Ready for testing in System Integration Nov 12, 2019
@brs17 brs17 moved this from Ready for testing to Testing in System Integration Nov 12, 2019
@jackpot51 jackpot51 merged commit 253a2c1 into master Nov 12, 2019
System Integration automation moved this from Testing to Done Nov 12, 2019
@jackpot51 jackpot51 deleted the dismisser branch November 12, 2019 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants