Skip to content

Use clearer dialog button texts + various translation improvements#7584

Merged
bramkragten merged 5 commits intohome-assistant:devfrom
spacegaier:dismiss-texts
Nov 9, 2020
Merged

Use clearer dialog button texts + various translation improvements#7584
bramkragten merged 5 commits intohome-assistant:devfrom
spacegaier:dismiss-texts

Conversation

@spacegaier
Copy link
Member

@spacegaier spacegaier commented Nov 3, 2020

Breaking change

Proposed change

Original scope of the PR: Currently the reload dialogs are Ok/Cancel which is weird since they suggest you can cancel the overall process e.g. the process of deleting a resource (after which this dialog pops up). This PR converts them to Yes/No.

New scope of the PR:
By now it has increased to include the following:

  • Use clearer dialog button texts throughout the frontend, e.g. move away from simple Yes/No to more descriptive versions such as Delete/Cancel, Enable/Cancel, Refresh/NotNow, Stay/Leave (for unsaved changes popups).
  • Add proper plural handling to the disable/enable/remove dialogs in the entity list
  • Consistently spell "URL" in capitals
  • Added a missing translation

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Copy link
Contributor

@adamjernst adamjernst left a comment

Choose a reason for hiding this comment

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

Nice tweak! I'm not a maintainer, just a drive-by commenter...

"ui.panel.config.lovelace.resources.refresh_body"
),
confirmText: this.hass.localize("ui.common.yes"),
dismissText: this.hass.localize("ui.common.no"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would prefer "Refresh" / "Ignore", because it makes it clear what the buttons do if you don't read the text. 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be fine with "Refresh", but I am not sure about "Ignore". In other places such as the integration config page, "Ignore" means really ignore from now on out = do not ask again. That however is not the case here and therefore could be confusing plus the user is not really ignoring it here (in the sense of stopping e.g. a longer guided "wizard" / dialog), but deciding "no" right on this popup.

Copy link
Member

Choose a reason for hiding this comment

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

I like "Refresh" for the yes button. The no could be something as Not now

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed to "Refresh".

@bramkragten I am not sure if there really is a meaningful difference between "No" and "Not now". Isn't a "No" basically always implicitly a "Not now"? There are not many situations where you couldn't come back later and do whatever you previously declined. If there is no real difference I would stick with "No" since shorter and commonly (and consistently) used through the frontend today.

Copy link
Contributor

Choose a reason for hiding this comment

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

@spacegaier I myself was influenced by the Apple Human Interface Guidelines, which say that you should avoid using Yes and No for button titles.

You point out that No is already widely used in the frontend, but hey, it's never too late to start doing better :D

I vote for "Not Now" myself, because it is clear what the dialog will do even if you only read the button titles.

Copy link
Member Author

@spacegaier spacegaier Nov 7, 2020

Choose a reason for hiding this comment

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

I now went through the code base and applied this principle of more meaningful button texts to all dialogs. Got rid of yes/no in most places and substituted by clearer texts, e.g. delete/cancel or enable/cancel or leave/stay (for unsaved changes popups). The concrete refresh topic that started this PR is no refresh/not_now.

Let me know what you think.

Note: I saw we have both "delete" and "remove" used. It makes sense e.g. for tags to only be removed since we are actually deleting the physical tag, but in other areas that distinction might be not really thought through yet.

if (
!(await showConfirmationDialog(this, {
title: this.hass!.localize("ui.panel.config.zone.confirm_delete"),
text: this.hass!.localize("ui.panel.config.zone.confirm_delete2"),
Copy link
Member Author

Choose a reason for hiding this comment

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

This "text" element did not actually exist, so I only kept the title.

@spacegaier spacegaier changed the title Convert misleading "cancel" dialogs to yes/no Use clearer dialog button texts + various translation improvements Nov 7, 2020
"back": "Back",
"undo": "Undo",
"save": "Save",
"rename": "Rename",
Copy link
Member

Choose a reason for hiding this comment

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

This one is used once, not really common?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it in here, since I interpreted "common" to also also mean "generic" => it doesn't make sense to stick this really generic simple string into a specific subsection IMHO.

I wouldn't be surprised if other processes in the future would require the same as well.

@bramkragten bramkragten merged commit 5a9bd73 into home-assistant:dev Nov 9, 2020
@spacegaier spacegaier deleted the dismiss-texts branch November 9, 2020 22:46
KTibow added a commit to KTibow/frontend-1 that referenced this pull request Nov 10, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants