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

Desktop: Make “update is available” dialog box easier to use #3877

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

roryokane
Copy link
Contributor

Electron docs for dialog.showMessageBox: https://www.electronjs.org/docs/api/dialog#dialogshowmessageboxbrowserwindow-options

  • In the message, fix a comma splice (grammar error) and remove some unnecessary words.

  • Split the message into message and detail parts.

  • Use specific verbs for the button labels. This follows the UI guidelines of major operating systems, including macOS and Windows. (cancelId was added while testing out different button labels that Electron might not automatically recognize as a cancel button. I think the cancelId is worth keeping to ensure the Esc key still works if the label is changed in the future.)

Screenshots

Old update dialog box as seen in v1.0.241:

old dialog in v1 0 241

New update dialog box with this change, with long release notes:

new dialog with long, truncated release notes

New update dialog box with short release notes:

new dialog with short release notes

New update dialog box with no release notes:

new dialog with no release notes

Electron docs for `dialog.showMessageBox`: https://www.electronjs.org/docs/api/dialog#dialogshowmessageboxbrowserwindow-options

- In the message, fix a comma splice (grammar error) and remove some unnecessary words.

- Split the message into `message` and `detail` parts.

- Use specific verbs for the button labels. This follows the UI guidelines of major operating systems, including macOS and Windows. (`cancelId` was added while testing out different button labels that Electron might not automatically recognize as a cancel button. I think the `cancelId` is worth keeping to ensure the Esc key still works if the label is changed in the future.)
@laurent22
Copy link
Owner

Thanks, it's a good change to rename "Yes" to "Download", but can we limit the pull request to that? In particular you've changed strings which means they will all have to be translated again, even though the new text is not obviously better.

@laurent22
Copy link
Owner

As for the "details" property, can you test on all platforms including Windows and Linux? If not, it's best to leave as it is as we don't know how it's going to look or if it will work at all.

@roryokane
Copy link
Contributor Author

roryokane commented Oct 7, 2020

it's a good change to rename "Yes" to "Download", but can we limit the pull request to that?

Yes, I can change this pull request or split out a new pull request with whatever changes you’re willing to accept.

Dialog message changes

the new text is not obviously better

Many of these changes are a matter of taste, yes. One of the changes is an objective improvement, though: fixing the comma splice at the top of the message. If I wanted to change the text as little as possible, I should at least make this change:

-An update is available, do you want to download it now?
+An update is available. Do you want to download it now?

My other change to the dialog’s message—shortening the second sentence to “Download it now?”—is minor and I can let it go.

Dialog button changes

As for the buttons, if we accept the change from “Yes” to “Download”, I was suggesting changing the buttons from this:

Full Release Notes, No, Download

to this:

Open Full Release Notes, Cancel, Download

Though these two changes are subjective, they are supported by the human interface guidelines for macOS, Windows, and Gnome, which advise using verbs for buttons and avoiding Yes/No:

Quotes from interface guidelines (click to expand)

Apple’s macOS interface guidelines on push buttons:

Use verbs in push button titles. An action-specific title shows that the button is interactive and conveys what happens when clicked. For example, Save, Close, Print, Delete, and Change Password are action-specific titles.

Apple’s macOS interface guidelines on alerts:

Give alert buttons succinct, logical titles. The best button titles consist of one or two words that describe the result of clicking the button. […] To the extent possible, use verbs and verb phrases that relate directly to the alert title and message—for example, View All, Reply, or Ignore. […] Avoid using Yes and No.

Windows UI guidelines on dialogs:

Use specific responses to the main instruction or content as button text. An example is, "Do you want to allow AppName to access your location?", followed by "Allow" and "Block" buttons. Specific responses can be understood more quickly, resulting in efficient decision making.

Gnome interface guidelines on buttons:

Label all buttons with imperative verbs, using header capitalization. For example, Save, Sort or Update Now.

Another reason I think “No” should be changed is that a “No” option without a “Yes” option looks weird to me.

Given this, do my textual changes seem better to you? Or perhaps you would prefer “Ignore” or “Close” instead of “Cancel”, which I had also considered?

Use of the detail property

As for the "details" property, can you test on all platforms including Windows and Linux? If not, it's best to leave as it is as we don't know how it's going to look or if it will work at all.

Sorry, I can’t test on Windows and Linux. However, I found this evidence that detail is supported on other platforms:

Thus, I think the detail property is safe to use.

@laurent22
Copy link
Owner

For Electron, I'm not trusting any API that hasn't been tested so we can't use the "details" parameter unfortunately. We had too much troubles with the Electron API in the past so if something works I prefer to leave it as it is. I'm afraid we also won't change the comma to a colon as that would invalidate the 30+ translations.

@roryokane
Copy link
Contributor Author

roryokane commented Oct 7, 2020

I'm afraid we also won't change the comma to a colon as that would invalidate the 30+ translations.

OK. It surprises me that your translation system can’t migrate the old translations of updated text, but if that’s the system you’re stuck with I can see why you would dislike updating text.

There’s one of my suggestions that you didn’t respond to, and which wouldn’t require adding any new translations: changing the “No” button to “Cancel”. The files in ElectronClient/locales/ already have translations for “Cancel”, so the only factor in the decision should be which button label is clearer.

In my last comment I gave additional arguments for “Cancel”/“Download”: adherence to cross-platform interface guidelines that buttons should contain verbs, and avoiding the confusion of having “No” without “Yes”. Knowing that, do you still want to make the labels “No”/“Download”?

@mcorossigo
Copy link

Sorry, I can’t test on Windows and Linux.

Would it be sufficient if I build this PR on both Linux and Windows and then post a screenshot of the resulting dialog?

@roryokane
Copy link
Contributor Author

roryokane commented Oct 9, 2020

@mcorossigo – If @laurent22 will accept that test method, I would definitely appreciate you doing that.

If you do that, you’ll need to (edit) test using this branch: roryokane:improve-update-available-dialog-testing, which includes the following change that allows the dialog to show up when choosing “Check for updates…” from the menu:

Code that allows the relevant dialog to be displayed (included in the linked branch)
--- a/ElectronClient/checkForUpdates.js
+++ b/ElectronClient/checkForUpdates.js
@@ -131,7 +131,7 @@ function checkForUpdates(inBackground, window, logFilePath, options) {
                autoUpdateLogger_.info(`Latest version: ${release.version}`);
                autoUpdateLogger_.info('Is Pre-release:', release.prerelease);
 
-               if (compareVersions(release.version, packageInfo.version) <= 0) {
+               if (false) {
                        if (!checkInBackground_) {
                                await dialog.showMessageBox({
                                        type: 'info',

Also, if you’re curious what different release notes would look like, you can use the following code. It’s optional, though—we can test whether detail works cross-platform without it.

Optional code for displaying custom release notes in the dialog
--- a/ElectronClient/checkForUpdates.js
+++ b/ElectronClient/checkForUpdates.js
@@ -140,6 +140,7 @@ function checkForUpdates(inBackground, window, logFilePath, options) {
                                });
                        }
                } else {
+                       release.notes = "example short release notes" // for testing
                        const fullReleaseNotes = release.notes.trim() ? `\n\n${release.notes.trim()}` : '';
                        const MAX_RELEASE_NOTES_LENGTH = 1000;
                        const truncateReleaseNotes = fullReleaseNotes.length > MAX_RELEASE_NOTES_LENGTH;

@laurent22
Copy link
Owner

@mcorossigo, yes if you can try that I would also accept this test method.

@mcorossigo
Copy link

mcorossigo commented Oct 12, 2020

Here the result for Windows 10 2004 and Ubuntu 20.04 (Gnome) (click on the thumbnails for the large version):

OS Current New Long New Short
Windows 20201012_144554_29 20201012_143802_26 20201012_144225_27
Linux Schermata da 2020-10-21 22-10-29 Schermata da 2020-10-21 21-38-24 Schermata da 2020-10-21 21-40-20

The new dialog looks pretty good TBH.

…ating new text

with the exception of keeping the renaming of one button to “Download”, which was approved

per the discussion in laurent22#3877
@mcorossigo
Copy link

Hey, I finally have been able to build on Linux, and I've updated the comment with the screenshots.

@laurent22
Copy link
Owner

Looks good now, thanks for your work @roryokane, @mcorossigo!

@laurent22 laurent22 merged commit 5bc25ae into laurent22:dev Oct 22, 2020
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.

3 participants