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

[UX] Contact form categories: better messages #3215

Closed
klonos opened this issue Jul 28, 2018 · 12 comments
Closed

[UX] Contact form categories: better messages #3215

klonos opened this issue Jul 28, 2018 · 12 comments

Comments

@klonos
Copy link
Member

klonos commented Jul 28, 2018

Part of the #3023 META...

  1. When the "Recipients" text area contains more than one invalid email addresses, the validation message only tells the user about the first one that is not valid:

    screen shot 2018-07-28 at 10 11 55 pm

    I think we should either be listing all invalid addresses, or in the case where more than one, have the message be generic, like "Multiple invalid email addresses detected." or something.

  2. The success message is "Category xyz has been saved/deleted.". I think that it should be "Contact form category xyz has been saved." instead.

    screen shot 2018-07-28 at 10 33 38 pm screen shot 2018-07-28 at 10 45 22 pm

    ...especially for the watchdog message:

    screen shot 2018-07-28 at 10 46 32 pm

PR backdrop/backdrop#2257

@opi
Copy link

opi commented Jul 30, 2018

Creating PR backdrop/backdrop#2257 as a starter for discussion.

  1. Here are the 2 error messages, depending if there is one or more invalid addresses:

    • test@wrong is an invalid e-mail address.
    • test@wrong, more, error are invalid e-mail addresses.
  2. Totally agree

@klonos
Copy link
Member Author

klonos commented Jul 30, 2018

...the way this is currently implemented in the PR, we check whether there is one or multiple invalid email addresses. If we wanted to achieve "natural language", we should be having the message including an "and" between the previous to last and the last item. Something like this: https://stackoverflow.com/questions/8586141/implode-array-with-and-add-and-before-last-item

...but this complicates things even more code-wise. I think that it'd be better, and more readable if we said something like "The following are not valid email addresses:" ...and then we had the invalid ones in a list.

@opi
Copy link

opi commented Jul 30, 2018

Poll:

  1. "The following are not valid email addresses: aaa, bbb, ccc"
  2. "The following email addresses are not valid: aaa, bbb, ccc"

@klonos
Copy link
Member Author

klonos commented Jul 31, 2018

Either is fine by me.

@opi
Copy link

opi commented Jul 31, 2018

PR updated with this message: The following email addresses are not valid: %recipients..

@klonos
Copy link
Member Author

klonos commented Jul 31, 2018

Looks good to me, and you can ignore my next comment and consider this RTBC, but I get annoyed by the fact that we use "not valid" for multiple and "invalid" for one:

The following email addresses are not valid: ...
xyz is an invalid e-mail address.

Either change the first to The following email addresses are invalid: ..., or change the second to xyz is not a valid e-mail address..

I don't have a preference, either one is fine ...it's the inconsistency that bothers me. Sorry ...OCD 😄

@opi
Copy link

opi commented Jul 31, 2018

PR updated with The following email addresses are invalid: %recipients.. Mostly because '%recipient is an invalid e-mail address.' should be already translated in most cases.

@klonos
Copy link
Member Author

klonos commented Jul 31, 2018

Yep, RTBC 😉

@opi
Copy link

opi commented Aug 6, 2018

PR rebase against 1.x, and updated with a proper format_plural usage. Back to "need review" then.

@opi
Copy link

opi commented Aug 6, 2018

@serundeputy is this long line OK with coding standards ?

@herbdool
Copy link

Tested and looks good to me.

@opi that line doesn't look any longer than others nearby.

@quicksketch
Copy link
Member

To avoid unnecessary string changes in 1.10.x, I've merged backdrop/backdrop#2257 into 1.x only.

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Milestone
1.11.0
Development

No branches or pull requests

4 participants