feat(field): add custom error message and ng-pattern support #109
Conversation
JDownloader
force-pushed
the
feature/field-custom-error-messages
branch
from
346dddd to
0e32543
Compare
neolitec
changed the title
packages/oui-field/README.md
Outdated
|
|
||
| ### error-messages | ||
|
|
||
| | Attribute | Type | Binding | One-time binding | Values | Default | Description | |
There was a problem hiding this comment.
It's not really a tag attribute, so... some columns should not be there like "Binding", "One-time binding"...
Maybe it's could be sufficient to only show the default object and explaining that it can be overridden.
There was a problem hiding this comment.
I think it could be replaced with a configuration section like for pagination component.
An exemple of provider configuration with default message could be interesting
There was a problem hiding this comment.
Yeah, the binding is for the object containing the messages, I was was not sure on how to present it. I will take a look at the pagination section to try to improve that.
| minlength: ["minlength", "ng-minlength"], | ||
| maxlength: ["maxlength", "ng-maxlength"] | ||
| maxlength: ["maxlength", "ng-maxlength"], | ||
| custumValidation: ["pattern", "ng-pattern"] |
There was a problem hiding this comment.
custum -> custom
Or could it be simply named pattern so that it can be reused with {{pattern}} in error message?
|
|
||
| describe("with validation", () => { | ||
| it("should retrieve custom error messages", () => { | ||
| const message = "Username must be a least 6 characters."; |
There was a problem hiding this comment.
(optional but should not be) I don't remember if it has been done before but I think you should also test that interpolation works in custom messages.
There was a problem hiding this comment.
This could be done in a new test, only for testing the interpolation.
packages/oui-field/README.md
Outdated
| id="username" | ||
| name="username" | ||
| ng-model="$ctrl.user.username" | ||
| ng-pattern="/^[a-z]{3,8}$/"> |
There was a problem hiding this comment.
(optional) Maybe [a-zA-Z] to exactly match with the error message.
packages/oui-field/README.md
Outdated
|
|
||
| ## API | ||
|
|
||
| ### oui-datagrid |
There was a problem hiding this comment.
Why oui-datagrid? copy/paste error before your pull-request
There was a problem hiding this comment.
@tremblaymath I'm not what you want about that subtitle
There was a problem hiding this comment.
oui-datagrid has no sense in field component documentation.
To remove
| </oui-field> | ||
| <oui-field label="{{'Username'}}" | ||
| error-messages="{pattern: 'Username must be alphabetic with a length between 3 and 8.'}"> |
There was a problem hiding this comment.
You should add a helper text "Username must be alphabetic with a length between 3 and 8.", it will be helpful to trigger error. It could be interesting for other examples too.
There was a problem hiding this comment.
An helper text or an error, not both of them...
Maybe something like "This input is valid only if..." for helper text.
There was a problem hiding this comment.
I would avoid also doing both, I think the error is easy to trigger like the lastname field.
There was a problem hiding this comment.
easy to trigger yes!
But the user doesn't know the validations without clicking on "Show the example" and reading the code.
"I think the error is easy to trigger like the lastname field." There is already a help text for lastname "At least 3 chars" and it's a good thing that we should reproduce
packages/oui-field/README.md
Outdated
|
|
||
| ### error-messages | ||
|
|
||
| | Attribute | Type | Binding | One-time binding | Values | Default | Description | |
There was a problem hiding this comment.
I think it could be replaced with a configuration section like for pagination component.
An exemple of provider configuration with default message could be interesting
| minlength: ["minlength", "ng-minlength"], | ||
| maxlength: ["maxlength", "ng-maxlength"] | ||
| maxlength: ["maxlength", "ng-maxlength"], | ||
| custumValidation: ["pattern", "ng-pattern"] |
| }); | ||
| }); | ||
|
|
||
| describe("with validation", () => { |
There was a problem hiding this comment.
Good test for custom message!
We should test default error messages too. Almost identical to this one
There was a problem hiding this comment.
Yeah, but it the test should not enforce a string but verify it's actually using the one from the provider
There was a problem hiding this comment.
Yes I agree, provider should be mocked and we should only "verify it's actually using the one from the provider"
There was already missing tests before you started
| </oui-field> | ||
| <oui-field label="{{'Username'}}" | ||
| error-messages="{pattern: 'Username must be alphabetic with a length between 3 and 8.'}"> |
There was a problem hiding this comment.
I would like to see parameters in this example
tremblaymath
dismissed
neolitec’s stale review
No blocking comments. Doc and tests could be improved after
tremblaymath
force-pushed
the
feature/field-custom-error-messages
branch
from
8b9b923 to
482a8fa
Compare
No description provided.