-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: add default variant to Alert and Badge #5456
Conversation
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.
thoughts folks? I think this is probably reasonable consistency change, however i do think it'd have to be considered a breaking change so maybe (if accepted) should go in the v5 branch instead?
This bugfix aligns the `<button>` variant class implementation with Alert/Badge/Navbar -- only outputing a `prefix-variant` formatted class when a variant is specified and avoiding the situation of outputting `.btn-null` or `.btn-` when the variant is null or an empty string respectively Split from react-bootstrap#5456
3318a90
to
4bc3e13
Compare
This bugfix aligns the `<button>` variant class implementation with Alert/Badge/Navbar -- only outputing a `prefix-variant` formatted class when a variant is specified and avoiding the situation of outputting `.btn-null` or `.btn-` when the variant is null or an empty string respectively Split from #5456
Sounds good to me. Let's push it to v5 |
I would agree on that as well. |
@davidjb, sorry for the delay on this. Can you rebase this and change Thanks! |
This sets a default variant and bg of "primary" for Alert and Badge respectively, allowing them to be used without having to set a variant or bg. Bootstrap's docs on Alerts require the use of a contextual class (https://getbootstrap.com/docs/5.1/components/alerts/) in order to have proper styling. This aligns Alert and Badge variant functionality with that of Button and Navbar. This could be viewed as a breaking change (e.g. for someone using Alert without a variant [as it now adds one] or someone relying on Button's `.btn-null` [if the variant was null]). However, I'm inclined to view this as a feature as it is adding & declaring functionality in the public API where behaviour wasn't previously explicitly defined in examples or docs.
4bc3e13
to
f5bf34b
Compare
@kyletsang Done! That should be it - rebased, updated the Badge and its tests, plus updated commit message. In v5.x, the Badge is particularly important to have a default bg because its text is white, making it unreadable on a standard background. At least in 4.x it was black text by default. |
Thanks! |
This sets a default variant of "primary" for Alert and Badge, allowing them to be used without having to set a variant. Bootstrap's docs on Alerts/Badges require the use of a contextual class (https://getbootstrap.com/docs/4.5/components/alerts/) in order to have proper styling.
This aligns Alert and Badge variant functionality with that of Button and Navbar, where default variants are provided and can be used like
<Button>Hello</Button>
.This could be viewed as a breaking change (e.g. for someone using Alert without a variant as it now adds one). However, it could be viewed as a feature as it is adding & declaring functionality in the public API where behaviour wasn't previously explicitly defined in examples or docs.