Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ValidationRules): add number validation rules #519

Merged
merged 2 commits into from
Mar 21, 2019

Conversation

CuddleBunny
Copy link
Contributor

Add common validation rules for numeric properties.

resolves #440

Add common validation rules for numeric properties.

resolves aurelia#440
@CLAassistant
Copy link

CLAassistant commented Feb 20, 2019

CLA assistant check
All committers have signed the CLA.

@CuddleBunny
Copy link
Contributor Author

I am unsure about a proper validation message for range. I wish English had a word that meant the same as "between, inclusive." I suspect this is probably okay though because a custom message can be used when this doesn't suffice.

@bigopon
Copy link
Member

bigopon commented Feb 20, 2019

@CuddleBunny Nice. Maybe it will cause a big confusion without clear documentation, can you help add something too. Also about that, should there be rangeInclusive, rangeBetween?
cc @fkleuver @EisenbergEffect

@CuddleBunny
Copy link
Contributor Author

@bigopon I can update the documentation, it has moved outside to aurelia/documentation right?

Splitting range into two rules is also a good idea. Perhaps just range and between?

@EisenbergEffect
Copy link
Contributor

@CuddleBunny Yes indeed. Docs are now here: https://github.com/aurelia/documentation/blob/master/current/en-us/7.%20plugins/3.%20validation.md Ping me on the docs PR and we'll get these things in together.

@bigopon
Copy link
Member

bigopon commented Feb 21, 2019

#518 should go before this @EisenbergEffect @fkleuver

@EisenbergEffect
Copy link
Contributor

@bigopon I've got #518 in now. Do this and all future PRs now need to target the develop branch?

@bigopon
Copy link
Member

bigopon commented Feb 23, 2019

I think so ... cc @fkleuver

@CuddleBunny CuddleBunny changed the base branch from master to develop March 7, 2019 17:58
@ricardograca
Copy link

Any reason for the documentation to show these when this Pull Request isn't even merged? I had to look in the source code to make sure I wasn't using those rules incorrectly.

@CuddleBunny
Copy link
Contributor Author

@EisenbergEffect - are we waiting on anything to merge this so that it matches the docs that have been deployed?

@EisenbergEffect
Copy link
Contributor

Apologies! I must have deployed the docs to update something else and forgot this was in there. We've got the dependency merged, so I'm merging this now. I'll work on getting a release out in the next day or two. As things look at the moment, probably tomorrow (Friday afternoon Pacific).

@EisenbergEffect EisenbergEffect merged commit 59150b9 into aurelia:develop Mar 21, 2019
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.

5 participants