-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
icon components #234
icon components #234
Conversation
Fixes #226 |
src/components/common/Icons.js
Outdated
color: PropTypes.string, | ||
size: PropTypes.oneOf(['small', 'medium', 'large']), | ||
iconTag:PropTypes.string, | ||
iconSize:PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can add a PropTypes.oneOf here similar to how you did the size proptype, but with the FontAwesome size variations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added propTypes options for iconSize.
src/components/common/Icons.js
Outdated
handleClick: PropTypes.func, | ||
color: PropTypes.string, | ||
size: PropTypes.oneOf(['small', 'medium', 'large']), | ||
iconTag:PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the word iconTag
to icon
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as requested.
src/components/common/Icons.js
Outdated
[`is-${size}`]: size, //for small, meduim, large, span tag | ||
[`has-text-${color}`]: color, | ||
[`fas-fa${iconSize}`]:iconSize //for fa-lg, fa-2x, fa-3x, icon tag | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add icon
to the classNames function so we can just add the name of the icon without the fa-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments to address before we push, but overall great job
Icon components have been created and ready for review