Skip to content

Conversation

@IvanScoles
Copy link
Collaborator

@IvanScoles IvanScoles commented Jul 27, 2018

Form validation - In progress

  • Form rules by array of fieds
  • Validation settings array objects with validation from each field, could be reused
  • Form validator class to manage validations by state

@IvanScoles IvanScoles requested a review from mravinale July 27, 2018 13:37
if (!this.canSubmitForm()) {
return;
}
let validation = this.validator.validate(this.state.user);
Copy link
Member

Choose a reason for hiding this comment

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

I dont see validation being reassingned, use const instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

const invalidPhone = validation.phone &&
!get(validation, 'phone.isValid');
const invalidSkypeId = validation.skypeId &&
!get(validation, 'skypeId.isValid');
Copy link
Member

Choose a reason for hiding this comment

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

Isn't a default return value for get needed? just in case

invalid={isNameInvalid}
feedback={errors.name}
invalid={invalidName}
feedback={get(validation, 'name.message')}
Copy link
Member

Choose a reason for hiding this comment

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

same here get(validation, 'name.message, '')

invalid={isEmailInvalid}
feedback={emailFeedback}
invalid={invalidEmail}
feedback={get(validation, 'email.message')}
Copy link
Member

Choose a reason for hiding this comment

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

Idem

placeholder="skype Id"
required={true}
invalid={invalidSkypeId}
feedback={get(validation, 'skypeId.message')}
Copy link
Member

Choose a reason for hiding this comment

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

Idem

placeholder="Phone Number"
required={true}
invalid={invalidPhone}
feedback={get(validation, 'phone.message')}
Copy link
Member

Choose a reason for hiding this comment

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

Idem

...validationSettings.email,
...validationSettings.skypeId,
...validationSettings.phone
]
Copy link
Member

Choose a reason for hiding this comment

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

I think we will need lodash cloneDeep here (unless you spread each nested object), spread will clone first level attributes. Nested Objects will be passed by reference and unexpectedly mutated.

yarn add lodash.cloneDeep

and here
import cloneDeep from 'lodash.cloneDeep'

"sass-loader": "^7.0.3",
"style-loader": "^0.21.0",
"uglifyjs-webpack-plugin": "1.2.5",
"validator": "^10.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Move this to dependencies, it's part of the code bundle.

import FormInput from '../../common/form/FormInput';

import { Row } from 'reactstrap';
import { get } from 'lodash';
Copy link
Member

@emanuelmetal emanuelmetal Jul 27, 2018

Choose a reason for hiding this comment

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

Replace this with
import get from 'lodash.get';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I try to do the same with lodash.cloneDeep but I am having some issues: warning webpack-bundle-analyzer > bfj-node4@5.3.1: Switch to the bfj package for fixes and new
features!

});

return { isValid: true, ...validation };
}
Copy link
Member

Choose a reason for hiding this comment

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

Add a semi colon

this.toggle();
});
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this callback to a method to simplfy this handler, and prefer use arrow callback.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants