zulip / zulip-desktop

Zulip Desktop client for Mac, Windows and Linux.
https://zulip.com/apps
Apache License 2.0
840 stars 422 forks source link

Settings QA: #566

Open rishig opened 5 years ago

rishig commented 5 years ago

Copying from https://chat.zulip.org/#narrow/stream/16-desktop/topic/settings.20qa. I expect almost all of these will be fast changes, so maybe as first step you can fix or disagree with all the easy ones (e.g. wording changes, quick style fixes), and then we can open new issue(s) with the remaining list.

Settings

Settings: General

Settings: Network

Settings: Organization

Settings: Shortcuts

akashnimare commented 5 years ago

In the Settings menus, sometimes the back button is active and works, sometimes it is active and doesn't work, and sometimes it is not active. I wasn't able to get reliable reproducers, but if you click reload, different settings tabs, and the back button in random orders, maybe you can repro it.

This one is the expected behaviour. It only gets activated when it could go backwards. Also, it's disabled for the Setting page.

The buttons could be nicer (e.g. rounded corners, on-hover behavior of Add and Choose is broken in the General tab)

This is done in the latest release.

image

akashnimare commented 5 years ago

Show Desktop Notifications -> Should be "Suppress desktop notifications" or "Block desktop notifications from Zulip" to match "Mute all sounds from Zulip" (that's also a more accurate description of the setting).

I think Show Desktop Notifications makes sense here since by default it's turned on and if we choose to rename it like "Block desktop notifications from Zulip" we need to play with the default notification setting also which I don't is a good idea.

(requires restart) -> it seems like it does the restart automatically? The manual proxy config also seems to do an automatic restart, and possibly Save whether or not you click Save

This is just to make sure the settings gets apply successfully, @abhigyank right?

Download location: maybe indent this?

IMO, this looks good without the indentation.

abhigyank commented 5 years ago

(requires restart) -> it seems like it does the restart automatically?

No, currently it doesn't. Need to quit the app and restart.

The manual proxy config also seems to do an automatic restart, and possibly Save whether or not you click Save

It doesn't do a restart. I suppose it does a hard reload on clicking save.

akashnimare commented 5 years ago

@rishig can you re-visit the setting page again and see if the changes are good enough? I think we have covered most of the points from the checklist.

rishig commented 5 years ago

cool, looks great!

The remaining things that stand out:

akashnimare commented 5 years ago

Yeah, we could change the notification wording but this should be done carefully since this could override the notification settings and complicate things. The changes will require -

akashnimare commented 5 years ago

Show Desktop Notifications -> Should be "Suppress desktop notifications" or "Block desktop notifications from Zulip" to match "Mute all sounds from Zulip" (that's also a more accurate description of the setting).

We could also rename the Mute all sounds from Zulip to Play notification sound or something so that it matches with the flow and it would be a bit less complicated I think.

akashnimare commented 5 years ago

The vertical space here is still a bit confusing

@rishig does this look good? image

kanishk98 commented 5 years ago

(requires restart) -> it seems like it does the restart automatically? The manual proxy config also seems to do an automatic restart, and possibly Save whether or not you click Save

@rishig can you tell me if this is still an issue on your end? I just checked these out on my system and I seem to be getting expected behaviour. (manual restart in the first case, second case requiring us to click save before saving and then automatically reloading the app)

rishig commented 5 years ago

Not sure how this got lost -- sorry!