-
Notifications
You must be signed in to change notification settings - Fork 18
Feature/multiple validators #28
base: master
Are you sure you want to change the base?
Feature/multiple validators #28
Conversation
…matic instantiating) vs old minor doc update
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
CLAs look good, thanks! |
b80407f
to
45e8e7e
Compare
added resolutions documentation error message markup added resolution demo page name adjusted formatting, trying to make diffing easier in github minimize diff
b0d17a9
to
6e665ff
Compare
As much as I agree with a general need for applying multiple validations to an input or other control, I don't think this is the right approach. This and the others PRs seem like they have too much impact on @tlouisse, how would you feel about a simpler approach of creating a multi-validator. It would just implement <multi-validator id="composite-animal-validator">
<no-cats />
<no-dogs />
<ungulates-only-even-toed />
<only-feminine-name novalidate$="{{isBird}}" />
</multi-validator>
<paper-animal-input validator="composite-animal-validator" /> This way gives a cleaner API in my opinion and more flexible control over conditional validation based on each individual validator's attributes. It would also require no changes whatsoever in either validation behavior nor in any validatable control. |
Hi Tomasz, I don't know why there hasn't been any response to these PRs. It might be indeed because of the size and the therefore involved risk of losing backwards compatibility. In the company I work for we already use these pull requests (we have forked versions of the form elements) with success. If you worked with AngularJS forms before(they are quite good), you might be familiar with the above mentioned concepts. <input value="{{model.x}}" required custom-validator-a custom-validator-b /> I don't agree with your approach being simpler. The end goal here is to make it as easy as possible for the implementing developer to add several validators, without writing lots of boilerplate code. <only-feminine-name novalidate$="[[isBird]]" for="myInput"/>
<paper-animal-input validator="no-cats no-dogs only-feminine-name" validatable-id="myInput"/> In case the 'only-feminine-name' didn't need a custom configuration, it would only be one line (in your case it would remain 7 lines): <paper-animal-input validator="no-cats no-dogs only-feminine-name"/> Additionally I want to say that it's also a bit cumbersome that the default Polymer elements force you to keep track of why your input is invalid yourself (you need to write a distinct combined validator that keeps track of these states, see the "Before": https://tlouisse.github.io/iron-validatable-behavior/components/iron-validatable-behavior/demo/ As a developer, when I need to show a message, I do not only want to know that my input is wrong, I also want to know the reason why. In my pull request, there will be an 'errors' array in the input that keeps track of the validators being in error state. As a side note: ideally, for me, the API would have been like this: <paper-animal-input validators='["no-cats", "no-dogs", "only-feminine-name"]'/> Having the validator list as an array would be easier to reason about and prevents you from writing boilerplate that converts strings to arrays and vice versa. Hope to hear your thoughts about the above... Regards, |
Thank you Thijs, for such an elaborate response. By no means, either way the two approaches are not exclusive. And certainly they aren't exactly equivalent. My main point though is that creating an independent composite validator would allow integrating with current state of Polymer with any modifications required and any PRs being merged. With regard to simplicity, I'm not sold on the approach where you have multiple values in an attribute or an array. Could be a matter of taste but I think it would not play nice with the declarative nature of Web Components. Instead I expect you'd quickly have to resort to computed properties and in general I find such markup less visible. Regarding the less code argument, I have similar reservations. My proposed approach may be verbose but again it's also much more clear on what's happening. You would directly set up validators for a given field, annotate individually with And again, no changes necessary to any
I fully agree on both counts. I merely proposed an element to combine validators. How error messages are attached is a different matter regardless of the multi-validator functionality. It would deserve its own pull request to |
Oh and about attaching error messages, I just that had this idea that as a soft alternative you could simply use binding. Give a validator, you'd bind it between the validator and the validate field. Not ideal, but I'm following the path of least resistance <multi-validator id="composite-animal-validator" error-message="{{validationError}}">
<no-cats />
<no-dogs />
<ungulates-only-even-toed />
<only-feminine-name novalidate$="{{isBird}}" />
</multi-validator>
<paper-animal-input validator="composite-animal-validator" error-message="[[validationError]]" /> Such could support some of the functionality you mention about errors. |
what about something like this <paper-input bind-value="{{testmodel}}" value="{{testmodel}}">
<validator model="{{testmodel}}" rules="['no-dogs','no-cats','only-feminine-name','ungulates-only-even-toed']">
<validate name="no-dogs">custom no dogs message</validate>
<validate name="no-cats">custom no cats message</validate>
and more....
</validator> take a look at https://docs.angularjs.org/api/ngMessages/directive/ngMessages |
I agree that this approach (having a wrapper around multiple validators) would work without changes to the Polymer code base. The main point of mentioning the other areas the Polymer components lack, was pointing out that the Polymer form elements are too basic for us(and additions to their core seemed to be needed anyway.) If not altering/adding to existing Polymer would be the goal, your approach could be an option(there still would be a problem with native validators and error messages though, see below). <form is="iron-form">
<paper-input validator="custom-a custom-b custom-c"/>
<paper-input validator="custom-a custom-c"/>
<custom-unique configuration-param="[[_somethingUnique]"/>
<paper-input validator="custom-a custom-unique"/>
</form> Over this: <form is="iron-form">
<multiple-validator id="combinationOfValidators1" error-message="{{_errorM1}}">
<custom-a/>
<custom-b/>
<custom-c/>
</multiple-validator>
<paper-input validator="combinationOfValidators1" error-message="[[_errorM1]]"/>
<multiple-validator id="combinationOfValidators2" error-message="{{_errorM2}}">
<custom-a/>
<custom-c/>
</multiple-validator>
<paper-input validator="combinationOfValidators2" error-message="[[_errorM2]]"/>
<multiple-validator id="combinationOfValidators3" error-message="{{_errorM3}}">
<custom-a/>
<custom-unique configuration-param="[[_somethingUnique]"/]>
</multiple-validator>
<paper-input validator="combinationOfValidators3" error-message="[[_errorM3]]"/>
</form> I think the first one is at least equally declarative, with a whole lot less code. About the error messages: The approach (passing through the errorMessage) would work for custom validators, but how to integrate them with native validators(required, maxlength etc.)? |
@vospascal: <input is="iron-input" validator="vali-a vali-b" errors="{{_errors}}"/>
<div hidden$="[[!_errors.valiA]]">[[_errors.valiA.message]]</div>
<div hidden$="[[!_errors.valiB]]">My custom message</div> (You could combine this with paper-input-container to make whatever UI you want) You could also write two web-components to do it like this (or another API you might prefer): <poly-messages for="[[_errors]]">
<poly-message when="valiA">custom message A</poly-message>
<poly-message when="valiB">custom message B</poly-message>
</poly-messages> |
One problem with that is validator on the paper input you Should put the rules as a argument on the Poly-messages and write the validation as a component If there are errors fill the error property on the input marking iT invalid this maken it more easy to test validation rules and server side validation ill take a look later on How to syntax that properly stay tuned |
Currently, it's not possible to use multiple validators on an input.
Also see:
With this pull request, it will become possible to add multiple validators to elements implementing the validatable behavior, without having to write a new custom validator that combines functionality of other validators.
Along with this functionality, the following will be supported:
Everything is tested for backwards compatibility. All tests have run in chrome 51, firefox 46, OS X 10.9 safari 7, OS X 10.10 safari 8, OS X 10.11 safari 9, Windows 7 IE 10, Windows 8.1 IE 11, Windows 10 microsoftedge, Android 4.3, Android 4.4, Android 5, Android 6, iOS 7, iOS 8, iOS 9.3.
Note that, since the validation functionality is located in different repositories, the above functionality will contain pull requests in the following repositories:
The link below shows an example of what would be the benefit of the old vs. the new situation.
It makes clear what the API will look like and how little code will be needed to achieve the same in the new situation.
https://tlouisse.github.io/iron-validatable-behavior/components/iron-validatable-behavior/demo/