twbs / bootstrap

The most popular HTML, CSS, and JavaScript framework for developing responsive, mobile first projects on the web.
https://getbootstrap.com
MIT License
170.88k stars 78.88k forks source link

Cannot customize button border color #23450

Closed dozer75 closed 7 years ago

dozer75 commented 7 years ago

In the current _buttons.scss file, the same color is sent in as background and border color. This was not the case earlier where we could specify in our customization specific border and background colors.

We need to be able to customize border colors separate from the background. Otherwise why have borders at all?

I find this related to #23418

andresgalante commented 7 years ago

Although the border is defined at the mixin: https://github.com/twbs/bootstrap/blob/v4-dev/scss/mixins/_buttons.scss#L9

It's set as the same color of the background here: https://github.com/twbs/bootstrap/blob/v4-dev/scss/_buttons.scss#L55

I think this is a bug. The solutions I can think of are:

What do you think?

wojtask9 commented 7 years ago

I described the same problem here https://github.com/twbs/bootstrap/issues/23126

dozer75 commented 7 years ago

@andresgalante In my opinion this is one of the weakness with the new variable system. It is structured a lot better than earlier, but it isn't as flexible as it was, limiting us to either accept the default variations of basic color settings or to be forced to create our own definitions of the classes which I think will create more confusion, larger style sheets, more error prone and harder to read.

As for your suggestions, I think that the first one isn't an option. In our case, we want the background to be one color and the outer to be a completely other one ($white background and $blue border) and as far as I understand your suggestion it won't give me that.

The only way this can be solved for the usage I have is to be able to configure colors by themselves, and the only way is to have variables. However, instead of having lots of key/value pairs, why not use key/array structures instead? Like this: (I do not know scss that well, so just look at this example as pseudo code)

$buttons = (
    primary: (
        color: $white,
        background: theme-color("primary"),
        border: theme-color("primary"),        
        /* and other button related parameters */
   )
)

Then generate buttons based on this with:

@each $color, $values in $buttons {
  .btn-#{$button} {
    @include button-variant($values);
  }
}

and let button-variant read the configuration from $values?

As I said, it may not work since I don't know scss that well, but if it works it's worth a try.

I did get the issue described by @wojtask9 today and was planning to check it out tomorrow but it is clearly already registered. But I think that a similar construct like this (if it works) could be applied to lots of the similar issues.

andresgalante commented 7 years ago

@dozer75 you are absolutely right it is limiting and my solution wouldn't help much. A better mapping can be the solution but I am afraid that the more scss abstract we generate the harder it would be for people to understand what's happening under the hood and reduce contributions. The beauty of bootstrap is that it is easy to read in a way.

dozer75 commented 7 years ago

@andresgalante Actually I think that this abstraction will make it easier to read because users of the scss wont need to go into the source to find out why for example a button get's a specific border or foreground color or an alert get's it's border color or foreground color as we have to do now. By having it structured in the variables files with logical variable names it can be seen there. I have used two full days now with the current scss files to try to understand what's happening under the hood and it is quite confusing with all these auto calculated variants inline in functions. On the plus side it has given (or forced :) ) me the opportunity to learn scss more in-depth than I ever had before.

andresgalante commented 7 years ago

You are right @dozer75 I think it's worth the try, but it's not really my call 😄

If you do it I would love to review that PR and learn from it.

adampatterson commented 7 years ago

I found this because after updating from Alpha 6 to Beta I noticed that my own Button variations looked completely messed up.

It appears as though I can't directly control the text colour because of color-yiq and I can't find any documentation on how that works so I'm kind of stuck.

Even having $brand-primary removed seemed like something that should have happened well before alpha 6.

I feel like the mixins still need a lot more inline documentation.

DaleMckeown commented 7 years ago

color-yiq is define in _functions.scss.

@mixin color-yiq($color) {
  $r: red($color);
  $g: green($color);
  $b: blue($color);

  $yiq: (($r * 299) + ($g * 587) + ($b * 114)) / 1000;

  @if ($yiq >= 150) {
    color: #111;
  } @else {
    color: #fff;
  }
}

it works by contrasting the RGB values. If the output is >=150, the contrast is too high and the text colour is set to 111.

It's a nightmare trying to override mixins in Bootstrap at the moment! #23499

dozer75 commented 7 years ago

@andresgalante My problem is that I don't have that much time, but I'll give it a try to structure something however not sure when I can get it done. There is a lot of parameters that would have to be restructured around. But maybe start small to get some idea.

andresgalante commented 7 years ago

@dozer75 before you embark in something like this, I'd wait for the blessing from one of the core team, specially if you don't have much time.

dozer75 commented 7 years ago

@andresgalante Ok, but I am a bit curious myself, so I will try to do a small prototype portion to see if it even is possible in a neat clean way :) It will also be a good experience anyway to be able to handle bootstrap in the future by learning more of the internals.

dozer75 commented 7 years ago

@andresgalante I pushed a branch to a bootstrap fork on my account: https://github.com/dozer75/bootstrap/tree/enhance-button-variables Don't want to PR it as it is just to show one (of many) solutions to the issue.

I can't find really a good way to do it, one is to push lots of variables into _variables.scss, but I found that it messed it up quite a lot. So I went for this one where the theme button maps was filled with default values if they doesn't exist.

Most of the changes are in mixins/_buttons.scss, but also in the ordinary _buttons.scss (just calling to the mixins in _buttons.scss) and _variables.scss (comments on how to customize).

ddevault commented 7 years ago

I just ran into this as well. The variable changes are really bad, I was looking forward to v4 and now I'm not.

mdo commented 7 years ago

Closing as dupe of #23418.

joshbuckley commented 6 years ago

@mdo I could be missing something but this is a different issue than the one you linked to when closing as a dupe. I checked #23418 and #23611 (which supposedly resolved the former), and didn't find a solution for customizing the button border color.

Should this ticket be reopened?

DaleMckeown commented 6 years ago

@joshbuckley I agree. Customising the border colour is a seperate issue to #23418.

trojan03 commented 6 years ago

Any progress here?