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(Input): fix size prop (#660) #662

Merged
merged 3 commits into from Oct 30, 2017
Merged

fix(Input): fix size prop (#660) #662

merged 3 commits into from Oct 30, 2017

Conversation

gergely-nagy
Copy link
Collaborator

No description provided.

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor things to better line up with reactstrap and not restricting to basic bootstrap implementation only.

src/Input.js Outdated

const propTypes = {
children: PropTypes.node,
type: PropTypes.string,
size: PropTypes.string,
bsSize: PropTypes.oneOf(['lg', 'sm']),
Copy link
Member

Choose a reason for hiding this comment

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

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 {
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/Input.js Outdated
@@ -72,11 +73,16 @@ class Input extends React.Component {
}
}

if (['lg', 'sm'].indexOf(attributes.size) > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

You should also delete size from attributes so it doesn't get passed down to the DOM in this case.

src/Input.js Outdated
@@ -43,6 +44,7 @@ class Input extends React.Component {
} = this.props;

const checkInput = ['radio', 'checkbox'].indexOf(type) > -1;
const isNotaNumber = new RegExp('(?!^\\d+$)^.+$');
Copy link
Member

Choose a reason for hiding this comment

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

/\D/g would work and is more clear. new RegExp('\\D','g') if you want to use new RegExp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@TheSharpieOne TheSharpieOne merged commit cc2bd13 into reactstrap:master Oct 30, 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