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

use computed.bool instead of directly messages length #670

Merged

Conversation

panthony
Copy link
Contributor

because computed.or does not coalesce to bool, we otherwise end up with the number of errors instead of true in isInvalid and isInvalidAndTouched

Signed-off-by: Anthony Pessy anthony.pessy@gmail.com

@miguelcobain
Copy link
Owner

Can you convert these to integration tests, please?

because `computed.or` does not coalesce to bool, we otherwise end up with the number of errors instead of true in `isInvalid` and `isInvalidAndTouched`
@miguelcobain
Copy link
Owner

Can we promote

hasErrorMessages: computed.bool('validationErrorMessages.length'),

to the Mixin

and then paper-input would just override isInvalid to account for native invalids?

@panthony
Copy link
Contributor Author

@miguelcobain In ValidationMixin we would have:

  hasErrorMessages: computed.bool('validationErrorMessages.length'),

  isInvalid: computed.bool('validationErrorMessages.length'),

And in paper-input:

  isInvalid: computed.or('hasErrorMessages', 'isNativeInvalid'),

Is that correct?

@miguelcobain
Copy link
Owner

miguelcobain commented Mar 16, 2017

I was thinking, for consistency

// validation-mixin.js
hasErrorMessages: computed.bool('validationErrorMessages.length'),
isInvalid: computed.reads('hasErrorMessages'),

and

// override validation mixin `isInvalid` to account for the native input validity
isInvalid: computed.or('hasErrorMessages', 'isNativeInvalid'),

Please include the comment to make it clearer.

@miguelcobain
Copy link
Owner

Thanks!

@miguelcobain miguelcobain merged commit 72999f8 into miguelcobain:master Mar 16, 2017
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.

None yet

2 participants