IBX-2360: Validation content type create/edit #334
Conversation
lucasOsti
force-pushed
the
validation_content_type
branch
from
8b2e74c to
2db9168
Compare
lucasOsti
force-pushed
the
validation_content_type
branch
from
7587578 to
e7cb52e
Compare
lucasOsti
changed the title
| @@ -253,7 +260,60 @@ | |||
| }) | |||
| .catch(ibexa.helpers.notification.showErrorNotification); | |||
| }; | |||
| const validateInput = (input) => { | |||
| const isInputValid = !!input.value; | |||
There was a problem hiding this comment.
maybe it would be better with sth like is Input not empty? as non empty input isn't necessary valid, I think
| } | ||
| }; | ||
| const validateForm = () => { | ||
| const inputsToValidate = editForm.querySelectorAll('.ibexa-input[required]:not([disabled]):not([hidden])'); |
There was a problem hiding this comment.
what would you say on adding this selecto to some const? you use it few times here
There was a problem hiding this comment.
I'm not a big fan of this type of variables but ok :)
| validateInput(input); | ||
| }); | ||
|
|
||
| for (const [fieldDefinitionIdentifier, inputsStatus] of Object.entries(fieldDefinitionsStatuses)) { |
| input.addEventListener('change', () => validateForm(input), false); | ||
| input.addEventListener('blur', () => validateForm(input), false); | ||
| input.addEventListener('input', () => validateForm(input), false); |
There was a problem hiding this comment.
| input.addEventListener('change', () => validateForm(input), false); | |
| input.addEventListener('blur', () => validateForm(input), false); | |
| input.addEventListener('input', () => validateForm(input), false); | |
| input.addEventListener('change', validateForm, false); | |
| input.addEventListener('blur', validateForm, false); | |
| input.addEventListener('input', validateForm, false); |
There was a problem hiding this comment.
And AFAICS validateForm does not take any parameter, so actually no need to pass input.
| @@ -80,7 +89,9 @@ | |||
|
|
|||
| const fieldGroupInput = fieldNode.querySelector('.ibexa-input--field-group'); | |||
| const removeFieldsBtn = fieldNode.querySelectorAll('.ibexa-collapse__extra-action-button--remove-field-definitions'); | |||
| const inputsToValidate = fieldNode.querySelectorAll(SELECTOR_INPUTS_TO_VALIDATE); | |||
There was a problem hiding this comment.
I guess we don't have sonar/eslint checks in this repo yet, but it seems that it overrides the variable from the upper scope?
| if (isEditFormValid && isInputEmpty) { | ||
| isEditFormValid = false; |
There was a problem hiding this comment.
I think that checking whether isEditFormValid is true is not needed. You are also checking isInputEmpty in validateForm - can we avoid checking it twice?
There was a problem hiding this comment.
solved in private conversation
| if (!fieldDefinitionsCount) { | ||
| isEditFormValid = false; |
There was a problem hiding this comment.
My guess is you put this check here instead of validateForm, because you wanted to display a noFieldsAddedError message, but for me, it is not intuitive to have this checkout outside validateForm.
Can it be put there maybe? Or maybe it is intuitive for you? 🤔
There was a problem hiding this comment.
solved in private conversation
|
|
||
| isEditFormValid = true; | ||
|
|
||
| inputsToValidate.forEach((input) => { |
There was a problem hiding this comment.
Question: could this be iterated field definition by field definition without fieldDefinitionsStatuses?
There was a problem hiding this comment.
solved in private conversation
Added form validation by javascript
Checklist:
$ composer fix-cs)