A11Y: Improve create account modal for screen readers (PR #14204)

Ah thanks I wasn’t sure. You don’t have a tool to autoformat these do you?

I agree this is a lot of params, and I would do this in another PR, but what would you suggest doing with these? This and the other computed property are doing complex validations across multiple fields. I don’t know how I could group them into one property, and this password validation mixin is used in a couple of different places.

What would your approach be if you had to do it?

You could try to group things in state objects:

const options = {
  passwordRequired,
  passwordMinLength,
  rejectedPasswords
}

const account = {
  email,
  username
}

Just from reading it, I don’t have much idea what this is doing, can we thing of a better name ?

maybe we go with screenReaderTitle now ?

I had to introduce this because of:

Some fields should fail silently and not display the reason message under certain conditions. For example, if the password field is empty, we do not want to show the “please enter a password” message straight away, only on submit. So I made this property when submitting the create account form to change the validation behaviour and show the reason always:

Yes sorry missed that; I think I replaced a11y_ in this file and not just a11y

I do have one :slight_smile:

find . -name '*.hbs' | xargs yarn prettier --write --parser=glimmer

Now I see your question, why dont we use it? Well we can talk of it on chat if you want :wink:

Yes I see… It’s probably related to the fact we use observers in accounts and it creates this kind of issues indeed. I doubt you can do better without heavy refactoring.

Noting for posterity I am going to look into this in another PR, as it will probably require some refactoring.

Looks good to me, thanks!

After taking a look at this I do not think it is worth the effort of refactoring it right now. Take emailValidation; it relies on accountEmail along with a few other properties, so then I put those into an emailData object the computed property ends up looking like this anyway, so I am not sure why this is better?

    // Check the email address
    @discourseComputed(
      "emailData.{accountEmail,rejectedEmails,serverEmailValidation,serverAccountEmail}",
      "forceValidationReason"
    )
    emailValidation(emailData, forceValidationReason) {

accountEmail is then referenced by username-validation and password-validation, as these mixins are used together, which in turn is then used by both create-account.hbs and invites-show.hbs. invites-show.hbs also uses its own emailValidation function which copies a lot but not all of the stuff from create-account.js, further complicating things.

Just seems like a lot of churn for not much benefit to me, so I am leaving this for now. @jjaffeux