Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Report validity state of all registration fields on any change #2672

Merged
merged 5 commits into from
Feb 26, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Feb 21, 2019

This passes the validity state of all fields to the consumer of
RegistrationForm via the onValdiationChange callback, instead of just the
most recent error.

In addition, we notify the consumer for any validation change, whether success
or failure. This allows old validation messages to be properly cleared. It also
allows the consumer to be aware of multiple validation errors and display the
next one after you have fixed the first.

In addition, this tweaks a few surrounding bits related to validation to ensure bad fields are clearly highlighted in the right order.

field-validity

Fixes element-hq/element-web#8769

Validation is meant to run in reverse order of the fields (so that the last
message emitted is for the first invalid field).
If password confirm is empty on blur, there's no reason to try validating it.
The user may just be tabbing through fields.
This passes the validity state of all fields to the consumer of
`RegistrationForm` via the `onValdiationChange` callback, instead of just the
most recent error.

In addition, we notify the consumer for any validation change, whether success
or failure. This allows old validation messages to be properly cleared. It also
allows the consumer to be aware of multiple validation errors and display the
next one after you have fixed the first.

Fixes element-hq/element-web#8769
Until we have better validation, let's at least ensure fields with errors are
properly marked via color.
@jryans jryans requested a review from a team February 21, 2019 14:52
@turt2live turt2live self-assigned this Feb 22, 2019
onFormValidationFailed: function(errCode) {
onFormValidationChange: function(fieldErrors) {
// Find the first error and show that.
const errCode = Object.values(fieldErrors).find(value => value);
Copy link
Member

Choose a reason for hiding this comment

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

ooi why not [0]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There can be null values for fields without errors, so we need the first non-null one. Maybe !!value or a comment would make it clearer...

Copy link
Member

Choose a reason for hiding this comment

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

js is also weird and doesn't break on [0] fwiw

> const testArray = [];
undefined
> testArray[0]
undefined

Copy link
Collaborator Author

@jryans jryans Feb 22, 2019

Choose a reason for hiding this comment

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

Ah, but it's not about the values array being empty, but instead the elements in it. For example, the values array might be [null, "bob", null], and we want "bob" here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Yup, that makes sense. A comment would be great :)

@turt2live turt2live assigned jryans and unassigned turt2live Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Passwords don't match' doesn't disappear even when they match
2 participants