-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Switch to custom properties for colorizing buttons #30500
Conversation
be1c401
to
4a26947
Compare
4a26947
to
f1297ef
Compare
&:focus, | ||
&.focus { | ||
color: var(--btn-accent-contrast-color); | ||
background-color: var(--btn-accent-color); | ||
border-color: var(--btn-accent-color); | ||
} | ||
|
||
&.active, | ||
&:active { | ||
color: var(--btn-accent-contrast-color); | ||
background-color: var(--btn-accent-color); | ||
border-color: var(--btn-accent-color); | ||
} |
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.
Those are the same, can't we have a single selector stack?
Looks very promising, makes a lot of sense. |
We need to fix #29422 (comment) first, since the focus states also need to change for inputs, otherwise we 'll have inconsistent focus styles. |
.btn-outline { | ||
color: var(--btn-accent-color); | ||
background-color: transparent; | ||
border-color: currentColor; | ||
|
||
&:hover { | ||
background-color: var(--btn-accent-color); | ||
border-color: var(--btn-accent-color); | ||
} | ||
|
||
&:focus, | ||
&.focus { | ||
color: var(--btn-accent-contrast-color); | ||
background-color: var(--btn-accent-color); | ||
border-color: var(--btn-accent-color); | ||
} | ||
|
||
&.active, | ||
&:active { | ||
color: var(--btn-accent-contrast-color); | ||
background-color: var(--btn-accent-color); | ||
border-color: var(--btn-accent-color); | ||
} | ||
} |
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.
If I am not mistaken, the approach would be something like,
.btn {
--btn-color: default here;
--btn-background-color: default here;
--btn-border-color: currentColor;
color: var(--btn-color);
background-color: var(--btn-background-color);
border-color: var(--btn-border-color);
}
.btn-outline {
--btn-color: change me;
--btn-background-color: change me;
--btn-border-color: change me;
&:hover {
--btn-color: change me;
--btn-background-color: change me;
--btn-border-color: change me;
}
&:focus,
&.focus {
--btn-color: default here;
--btn-background-color: default here;
--btn-border-color: currentColor;
}
&.active,
&:active {
--btn-color: change me;
--btn-background-color: change me;
--btn-border-color: change me;
}
}
the idea is that you focus more on swapping the values than overriding styles.
At least that is how I understand it.
border-color: currentColor; | ||
|
||
&:hover { | ||
background-color: var(--btn-accent-color); |
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.
If we put here --btn-accent-color
for hover background-color
and for color
library user can't make these values different without styles overwrite or setting own selectors with variables values.
For example I want to set color
as red
, but background-color
as green
I will need to write:
.btn-outline {
--btn-accent-color: red;
&:hover {
--btn-accent-color: green;
}
}
I think better do config just with set of variables without writing new nested selectors. What do you think?
And, btw, isn't there color will be the same as background color or not?
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.
If we use approach similar to #31708 with placing in SCSS styles only SCSS variables, but in SCSS variables set CSS variables, we could do:
// Styles
.btn-outline {
color: $btn-color;
&:hover {
background-color: $btn-hover-bg-color;
}
}
// Default variables, will do the same as in PR
$btn-color: var(--btn-accent-color) !default;
$btn-hover-bg-color: var(--btn-accent-color) !default;
// In application can changed to e.g.:
$btn-color: var(--btn-color);
$btn-hover-bg-color: var(--btn-hover-bg-color);
// In application custom styles - setting variant without knowing selectors structure
.btn-custom {
--btn-color: red;
--btn-hover-color-bg: green;
}
@each $color, $value in $theme-colors { | ||
.btn-outline-#{$color} { | ||
@include button-outline-variant($value); | ||
--btn-accent-color: #{$value}; |
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.
I'm a bit confused by the fact that the variables are set here. The nice thing about CSS variables are that they cascade through the DOM hierarchy, so that devs can set these e.g. at the :root
or some other ascendant of a button, and every button (and other element that was configured in the ancestor) below that ancestor will use the same theme - without having to use any descendant selectors for .btn
etc.
But if the variable is set here on the .btn
element, then it seems that wouldn't work, because it would always override what was set in an ancestor. So to make it work, define a theme for all descendant Bootstrap components at an ancestor element, it seems we'd have to use specific descendant selectors like .btn
again to override the variables. That kind of defeats the purpose of the variables mostly.
How I imagine it, components like .btn
would only read variables, and they are set e.g. on :root
or any ancestor by devs (and Bootstrap can provide defaults on those and integrate them there with the Sass variables).
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.
I have the same impression, looks like CSS custom properties here used to refactor code in a cool way, not to increase customization capabilities and flexibility...
So, it's improves only internal Bootstrap development experience, decreases bundles size, provides some additional usages of modifiers, but not improves customization.
@MartijnCuppens can you rebase this please? |
Closing as stale and with hella conflicts 😅 |
No description provided.