fix(text-field): custom validation #430
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. 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. |
hvolschenk
force-pushed
the
fix/text-field-custom-validation
branch
from
5e3d328
to
2586c48
Compare
|
CLAs look good, thanks! |
Codecov Report
@@ Coverage Diff @@
## rc7.0 #430 +/- ##
==========================================
+ Coverage 96.14% 96.16% +0.01%
==========================================
Files 55 55
Lines 1841 1849 +8
Branches 214 217 +3
==========================================
+ Hits 1770 1778 +8
Misses 71 71
Continue to review full report at Codecov.
|
moog16
changed the title
| @@ -79,6 +91,15 @@ export default class Input extends React.Component { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| if (isValid !== prevProps.isValid) { | |||
There was a problem hiding this comment.
this looks good and makes sense to me, but what are we solving. I ran your example in #421. I am confused what the issue was.
There was a problem hiding this comment.
The issue was/is that in the example there is some custom validation.
To make the validation message appear I needed to focus the field, blur it, focus it again, and then blur it a second time for the validation message to appear.
There was a problem hiding this comment.
gotcha...interesting that it doesn't show up on the screenshot tests. I added isValid={false} on the , and it worked as expected.
|
@hvolschenk I agree we should rework the API for validation. Did you ever take a look at this documentation: https://github.com/material-components/material-components-web-react/tree/master/packages/text-field/helper-text#input-validation? We just did some text field updates, and were going to rework part of MDC Web's text field after this is fixed: material-components/material-components-web#4054. Your changes in the componentDidUpdate make sense. Could you please update the documentation in both the helper-text Readme and the main text-field readme? Thanks for all your contributions! |
remove check for `setUseNativeValidation` method as it will always exist make sure `setUseNativeValidation` is available in all stubbed unit tests
|
@moog16 I have updated the helper-text |
| return ( | ||
| <HelperText | ||
| isValid={false} | ||
| isValidationMessage |
There was a problem hiding this comment.
I see what you're saying in #421. let's remove isValidationMessage or validation as well and make them the same. If you wanna make this a different task we can.
There was a problem hiding this comment.
Let's please solve this through #435 if that's all right.
|
@hvolschenk please also sign the PR |
…ng standards change double quotes to single quotes in all jsx examples use `isValid` state properly in custom validation example
|
I signed it |
|
Thanks! I changed the base to rc7.0. And will wait for #436 to pass, then merge it! |
fixes #421
This does fix the issue, I am just not sure if this is how you want it to be fixed 😊