Set all bootstrap color variables so normal bootstrap utilities work #120
Conversation
|
A preview of this branch is available at https://sul-dlss.github.io/component-library/preview-120. |
| --bs-danger-border-subtle: rgb(var(--bs-danger-rgb)); | ||
| --bs-danger-text-emphasis: rgb(var(--bs-danger-rgb)); | ||
| --bs-blue: var(--stanford-sky-dark); | ||
| --bs-indigo: #6610f2; |
There was a problem hiding this comment.
I'd really prefer we only pull in the colors we need to do the functions in colors/text.html. Let's keep this to just what we need as it makes maintenance easier.
There was a problem hiding this comment.
How do we know what colors we'll need? My hope is we don't have to think about it, and if we ask for a bootstrap color in our applications we get one of the branding colors.
| --stanford-digital-blue-rgb: 0, 108, 184; | ||
| --stanford-digital-blue-dark: #00588c; |
There was a problem hiding this comment.
Can we use rgb(var(--stanford-digital-blue-dark-rgb)) instead. Then there's no risk of getting out of synch.
There was a problem hiding this comment.
I think upstream bootstrap defines their colors explicitly/separately. I figure it's probably a performance thing, and for as often as we're changing branding colors we can expect either a PR author and reviewer to look at the line above or below and catch it 🤷♂️
| --stanford-digital-blue-dark-rgb: 0, 84, 143; | ||
| --stanford-digital-green-dark-rgb: 0, 111, 84; | ||
| --stanford-digital-red-dark-rgb: 130, 0, 0; | ||
| --stanford-digital-green: #008566; | ||
| --stanford-digital-red: #b1040e; | ||
| --stanford-digital-red-light: #e50808; |
There was a problem hiding this comment.
Is this used by anything in the component library? I'd prefer that if it's not used in any of our designs, we keep it out, even if it is in the current identity guide.
There was a problem hiding this comment.
And I'm the opposite. It seems trivial to add now, and really annoying to have an incomplete component library when we need it.
| @@ -14,5 +14,5 @@ | |||
|
|
|||
| :root { | |||
| --font-family-serif: "Source Serif 4", serif; | |||
| --bs-font-sans-serif: "Source Sans 3"; | |||
| --bs-font-sans-serif: "Source Sans 3", system-ui, -apple-system, "Segoe UI", roboto, "Helvetica Neue", "Noto Sans", "Liberation Sans", arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; | |||
There was a problem hiding this comment.
| --bs-font-sans-serif: "Source Sans 3", system-ui, -apple-system, "Segoe UI", roboto, "Helvetica Neue", "Noto Sans", "Liberation Sans", arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; | |
| --bs-font-sans-serif: "Source Sans 3", system-ui; |
system-ui is broadly supported. I can't think of a case where we'd need more than this.
There was a problem hiding this comment.
I believe I copied the upstream font stack and pre-pended our brand font. That bootstrap hasn't dropped the fallbacks seems like an indicator that it's maybe too soon?
library-hours uses
.text-primary(presumably among other utilities). As a consumer of the component library styles, I think I'd expect bootstrap's utility classes to just work (and not end up with a combination of Stanford digital blue and Bootstrap-default blue).In the first commit of this PR ("Import upstream variables"), I bring over all the bootstrap variables from upstream: https://github.com/twbs/bootstrap/blob/main/dist/css/bootstrap.css#L7-L126
before merging them with our previous overrides, hopefully for easier review.
While evaluating the merge, I think I ran into a couple bugs: