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

feat(outline): Make outline a separate prop #62

Merged
merged 2 commits into from
Jul 28, 2016

Conversation

TheSharpieOne
Copy link
Member

The outline for buttons and cards is now it's own props separate from the color value.
Based on #33 (comment)

This also adds additional tests around this new props.

Also:
Cards, unlike buttons, do not have a default color. Thus adding outline prop to a card without a color will do nothing.
Is this acceptable? Should it produce a warning/error? Should it default to 'card-outline-secondary' when outline is provided but no color is provided?

The outline for buttons and cards is now it's own props separate from the color value.
Based on reactstrap#33 (comment)
@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ee5fa1e on TheSharpieOne:feature/outline-prop into f617dc5 on reactstrap:master.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 7496c91 on TheSharpieOne:feature/outline-prop into b62ee01 on reactstrap:master.

@eddywashere
Copy link
Member

@TheSharpieOne thanks a ton for the prs!

I think this is acceptable behavior. I think card already has a solid style without background variants applied where as the default .btn kind of looks weird and isn't really shown in the docs.

re: warnings/errors. I'd love to throw warnings in dev mode that catch issues like this and not include those warnings in production builds.

@eddywashere eddywashere merged commit c65e952 into reactstrap:master Jul 28, 2016
@TheSharpieOne TheSharpieOne deleted the feature/outline-prop branch August 17, 2016 13:45
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