fix(Input): fix size prop (#660)#662
fix(Input): fix size prop (#660)#662TheSharpieOne merged 3 commits intoreactstrap:masterfrom gergely-nagy:master
Conversation
TheSharpieOne
left a comment
TheSharpieOne
left a comment
There was a problem hiding this comment.
Looks good, just a few minor things to better line up with reactstrap and not restricting to basic bootstrap implementation only.
| children: PropTypes.node, | ||
| type: PropTypes.string, | ||
| size: PropTypes.string, | ||
| bsSize: PropTypes.oneOf(['lg', 'sm']), |
There was a problem hiding this comment.
Don't limit this. If someone wants to extend bootstrap to add something like xl (or anything else) they should be able to just add their class and pass it here.
| @@ -72,11 +73,16 @@ class Input extends React.Component { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I'd do a regex check to see if it is a word/not a number to trigger this logic. It works with the custom sizes better.
| } | ||
| } | ||
|
|
||
| if (['lg', 'sm'].indexOf(attributes.size) > -1) { |
There was a problem hiding this comment.
You should also delete size from attributes so it doesn't get passed down to the DOM in this case.
| } = this.props; | ||
|
|
||
| const checkInput = ['radio', 'checkbox'].indexOf(type) > -1; | ||
| const isNotaNumber = new RegExp('(?!^\\d+$)^.+$'); |
There was a problem hiding this comment.
/\D/g would work and is more clear. new RegExp('\\D','g') if you want to use new RegExp
There was a problem hiding this comment.
Yes , this is much better. Sorry, i forgot this global flag . Thank you for taking your time for the review. I'll fix it right away.
2 participants
No description provided.