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

fix(text-field): allow for input isValid override #374

Merged
merged 4 commits into from
Nov 5, 2018

Conversation

moog16
Copy link

@moog16 moog16 commented Oct 25, 2018

fixes #356

@moog16 moog16 assigned moog16 and bonniezhou and unassigned moog16 Oct 25, 2018
@codecov-io
Copy link

codecov-io commented Oct 25, 2018

Codecov Report

Merging #374 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
+ Coverage   96.94%   96.94%   +<.01%     
==========================================
  Files          53       53              
  Lines        1833     1836       +3     
  Branches      211      212       +1     
==========================================
+ Hits         1777     1780       +3     
  Misses         56       56
Impacted Files Coverage Δ
packages/text-field/Input.js 95.23% <100%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e903dd5...52daea0. Read the comment docs.

@@ -117,7 +117,12 @@ export default class Input extends React.Component {
}

isBadInput = () => this.inputElement.current.validity.badInput;
isValid = () => this.inputElement.current.validity.valid;
isValid = () => {
if (typeof this.props.isValid === 'boolean') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're just checking to see if this prop exists, right?

if (this.props.isValid !== undefined)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought I'm not sure which way of putting it is less awkward, up to you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did typeof this.props.isValid === 'boolean', because we want it to be true or false. If its anything else (null, undefined, 12, 'abc', etc.) then it shouldn't be considered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually...I guess they would get a warning via PropTypes. I will change to your suggestion.

@moog16 moog16 added this to the 0.6.x milestone Oct 31, 2018
@moog16 moog16 merged commit 7872856 into master Nov 5, 2018
@moog16 moog16 deleted the fix/text-field/validation-override branch November 5, 2018 22:41
@moog16
Copy link
Author

moog16 commented Nov 5, 2018

Created material-components/material-components-web#4054 which will include another follow up PR once completed.

4cm4k1 added a commit to 4cm4k1/material-components-web-react that referenced this pull request Nov 6, 2018
…ial-components-web-react into fix/list/allow-alternate-tags

* 'master' of https://github.com/material-components/material-components-web-react:
  docs(menu-surface): open state & ref errors (material-components#412)
  fix(text-field): allow for input isValid override (material-components#374)
  feat(list): Add List Group and List Group Subheader (material-components#386)
  fix(top-app-bar): allow react elements in title (material-components#376)
  docs: update main component manifest on main Readme (material-components#394)
  docs: update roadmap (material-components#396)
  docs: Add web survey link in readmes (material-components#402)
  docs: update main README with CRA2 guidelines (material-components#393)
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.

3 participants