Skip to content

Conversation

@MartijnCuppens
Copy link
Member

Closes #27002

  • Moved transparent background to .btn
  • Removed background-image: none; from the button-outline-variant mixin. Buttons and links don't have a background image by default, I guess this was a line to override the gradient backgrounds in the past.

@MartijnCuppens
Copy link
Member Author

I've also set the color and hover color to $body-color. This makes a.btn and button.btn render exactly the same.

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

@MartijnCuppens just a question, is the color definition a fallback for cases when .btn is used without a modifier?

@MartijnCuppens
Copy link
Member Author

@andresgalante, yes.

And if a developer wants to extend the .btn functionality with their own modifying classes, they can trust that their buttons will render the same for <a> and <button>.

@mixin button-outline-variant($color, $color-hover: color-yiq($color), $active-background: $color, $active-border: $color) {
color: $color;
background-color: transparent;
background-image: none;
Copy link
Member

Choose a reason for hiding this comment

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

I believe the background-image: none was for overriding some browser defaults on the button. I can't recall which browser or version, but I recall having to set this in order for custom backgrounds to work properly. Looking around, seems fine now.

Copy link
Member

@mdo mdo Aug 24, 2018

Choose a reason for hiding this comment

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

Ah, yes:

background-image: none; // Reset unusual Firefox-on-Android default style; see https://github.com/necolas/normalize.css/issues/214

And it looks like it was then removed in Android Firefox, so safe to ship I think. necolas/normalize.css#214 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but do we support such old versions anymore? I too remember this issue but it was very old.

Copy link
Member Author

Choose a reason for hiding this comment

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

The .btn elements don't have a background-image either, that's why I assumed it was save to remove this.

@mdo mdo merged commit 97eea3b into twbs:v4-dev Sep 2, 2018
@mdo mdo mentioned this pull request Sep 2, 2018
@MartijnCuppens MartijnCuppens deleted the btn branch September 3, 2018 04:53
@shanehoban
Copy link

The default .btn background color was actually quite nice in my opinion, and I was using it over btn-secondary (too harsh). I've lost this after moving to 4.2.1 😢

Would definitely like to see the .btn default background color come back (although I've just edited my bootstrap to counter this).

#ececec, with #d8d8d8 on hover is nice.

@MartijnCuppens
Copy link
Member Author

@shanehoban you shouldn't use .btn without modifier classes. We did this because the backgrounds of a.btn and button.btn weren't the same and that could lead inconsistencies when the buttons are extended. Best solution probably is to use the button-variant() mixin to generate a custom button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants