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

Consider re-thinking 'dependent' #44

Open
joefitter opened this issue Jun 2, 2016 · 4 comments
Open

Consider re-thinking 'dependent' #44

joefitter opened this issue Jun 2, 2016 · 4 comments

Comments

@joefitter
Copy link
Contributor

joefitter commented Jun 2, 2016

https://github.com/UKHomeOffice/passports-form-controller/blob/master/lib/validation/index.js#L36-L50

The dependent option in fields config should be re-thought as the intended behaviour isn't clear from the option name. To me dependent suggests that the field will be included in the form depending on a previous answer given, not that all validators will only be executed depending on a previous answer. It should be noted in the documentation that only fields that are already set (previous steps or editing) will be taken into account.

I would suggest either moving the dependent config option to each individual validator:

'phone-number': {
  validate: [
    {
      validator: 'numeric'
    },
    {
      validator: 'required',
      dependent: {
        field: 'contact-method',
        value: 'telephone'
      }
    }
  ]
}
  • phone number always needs to be numeric, but is only required if user selected 'telephone' for contact method on a previous step

or perhaps renaming it to validateIf or something similar.

@gavboulton
Copy link
Contributor

I can see how this could be confusing. dependent fields are those used in progressive reveals/disclosures. This property is used to decide if a field should be validated. I thought it only took effect on the current form values and not all values.

It's something to consider anyway.

@joefitter
Copy link
Contributor Author

You're right - i didn't dig too deeply - it only takes current step values into account. In this case I'd suggest renaming it to validateDependentOn or validateIf or something similar? We have a requirement to include/exclude a field on a step/form dependent on a previous field value, and this to me seems more fitting to the term dependent

@joefitter
Copy link
Contributor Author

dependentValidation?

@joefitter
Copy link
Contributor Author

naming is hard

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

No branches or pull requests

2 participants