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

Feature/multiple validators #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tlouisse
Copy link

@tlouisse tlouisse commented Sep 8, 2016

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:

  • custom messages:
    • stored in validators (so there's a single source of truth, resulting in consistency between forms and less development work)
    • support for multiple error messages, ordered by priority (the most important message will be shown by the paper-input)
  • support for native validators('required', 'max', 'min', 'step', 'maxlength', 'minlength', 'pattern', 'emailtype'(type=email) and 'urltype' (type=url))
  • automatic instantiating of custom validators
  • possibility to connect unique instances of a validator to a validatable-element (when overriding of the default message will be needed)

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/

@googlebot
Copy link

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!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@tlouisse
Copy link
Author

tlouisse commented Sep 8, 2016

I signed it!

@googlebot
Copy link

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.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 8, 2016
@tlouisse tlouisse force-pushed the feature/multipleValidators branch from b80407f to 45e8e7e Compare September 12, 2016 08:56
added resolutions

documentation error message markup

added resolution

demo page name

adjusted formatting, trying to make diffing easier in github

minimize diff
@tlouisse tlouisse force-pushed the feature/multipleValidators branch 2 times, most recently from b0d17a9 to 6e665ff Compare September 12, 2016 11:49
@tpluscode
Copy link

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 paper- and iron- elements. Not to mention they have been sitting here for months.

@tlouisse, how would you feel about a simpler approach of creating a multi-validator. It would just implement Polymer.IronValidatorBehavior by wrapping multiple validators and running them all when necessary. Something like

<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.

@tlouisse
Copy link
Author

tlouisse commented May 6, 2017

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.
Everything is backwards compatible though: all existing tests keep running without altering them.

In the company I work for we already use these pull requests (we have forked versions of the form elements) with success.
Coming from an AngularJS world, the form functionality of Polymer seems to be quite basic(a good starting point though), therefore we extended the form elements in other areas as well.
For instance, we created functionality for interaction states (dirty and touched) on an input and form level. These are quite vital for creating good UX experiences(deciding when an error message needs to be shown).
Also, we created behaviors making it possible to create custom inputs that have formatters and parsers, allowing us to make a distinction between the 'view value' and the 'model value'.

If you worked with AngularJS forms before(they are quite good), you might be familiar with the above mentioned concepts.
The inspiration for multiple validators came from AngularJS as well (custom validators are attribute directives that need to be applied on input level). For instance:

<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.
With our approach, your example would only be these two lines of code (instead of your 7).

<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.
Since the error messages of the validators are stored in the validators, the paper-input automatically can show the one with highest priority(of course you can also create different UX scenarios by observing these errors).
This functionality will be harder to realize with your proposal as well.

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.
Since we strived for backwards compatibility without altering the current API, we kept the validator attribute with type 'String', it's a small difference.

Hope to hear your thoughts about the above...

Regards,
Thijs

@tpluscode
Copy link

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 [novalidate] or per-validator settings (error messages?).

And again, no changes necessary to any PolymerElements/* thus instantly allowing anyone to use the new functionality without resorting to running off an unmerged branch...

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. [...] This functionality will be harder to realize with your proposal as well.

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 IronValidatorBehavior (and IronValidatableBehavior?) so that custom validators can supply their error text. Maybe there is already work on that? I haven't looked.

@tpluscode
Copy link

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.

@vospascal
Copy link

vospascal commented May 7, 2017

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

@tlouisse
Copy link
Author

tlouisse commented May 7, 2017

I agree that this approach (having a wrapper around multiple validators) would work without changes to the Polymer code base.
I think the multi-validator component would do something like this(https://github.com/tlouisse/iron-validatable-behavior/blob/master/demo/no-cats-or-catfishes.html), but then in a generic way, with the validators as variables(btw, doing this via content projection might lead to timing issues with auto-validate in Polymer 2, due to the changed lifecycle timings in the V1 spec, but that's a different discussion).

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).
However, the goal was to make a simple, expressive and declarative API that would provide in this.
Again, when I make a form, I would still prefer this:

<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.)?
It would require the multi-validator to have knowledge about the $$('paper-input').inputElement (the iron-input in this case, but the autogrow-textarea for the paper-textarea for instance.)
Not saying that would be impossible in the end, but it would probably lead to fuzzy, scattered code.

@tlouisse
Copy link
Author

tlouisse commented May 7, 2017

@vospascal:
I like the idea of having a ng-messages like approach. It could probably be done like this:

<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>

@vospascal
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants