Issue #89: Style/SCSS review#91
Issue #89: Style/SCSS review#91andrew-c-tran merged 3 commits intoddev:masterfrom aManNamedJed:style-review
Conversation
…ng in Bootstrap 4 as Node package.
| "primary": $primary, | ||
| "secondary": $grey, | ||
| "light": $light, | ||
| ); |
There was a problem hiding this comment.
theme-colors is an interesting new addition to bootstrap that I haven't played with much. Looks interesting! Though if i'm understanding this correctly we're not actually using this anywhere are we? all the references to $primary for example in all of the other files are simply referencing $primary itself and not grabbing the color from the map no?
There was a problem hiding this comment.
You're right -- I am not using the map directly, but Bootstrap actually uses the $theme-colors map to create all of the buttons, alerts, etc., so that alert-primary would then use the new color.
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_alert.scss#L47-L51
There was a problem hiding this comment.
ah, interesting, that's pretty cool! in previous experiments of sass-maps, in the case of a whitelabeled/rebranded ui it became kind of a PITA to update say a primary button color, does TWBS 4 have a better way of handling that besides overriding the entire map? Meaning, can we easily override just $theme-colors('primary') in an overrides file or would we have to redefine the entire map again?
There was a problem hiding this comment.
You can declare your $theme-colors before you load in BS4's styles, and it will basically fill in the gaps for you:
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_variables.scss#L74-L84
Basically this is saying "If there is no $theme-colors map, then create one now and fill it in with all this information. If there actually is a $theme-colors map, then merge it in with this data, but override the defaults provided".
It's all about that !default flag.
There was a problem hiding this comment.
Here is a thread in Bootstrap's repo where the devs/designers go back and forth on this if you're interested: twbs/bootstrap#23260
| "webpack-dev-server": "^2.11.0" | ||
| }, | ||
| "dependencies": { | ||
| "bootstrap": "^4.0.0", |
There was a problem hiding this comment.
nice! i didn't realize v4 finally went stable last month. feels like it's been in beta forever!
|
|
||
| h1 { | ||
| font-size: 2rem; | ||
| } |
There was a problem hiding this comment.
r/em isn't super required here since it's a packaged electron app, but i'm alright with going with rem going forward in the event we port to web.
andrew-c-tran
left a comment
andrew-c-tran
left a comment
There was a problem hiding this comment.
Looks good, i'm more than happy to pull this in.
|
|
||
| .btn { | ||
| cursor: pointer; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Thanks for modularizing these! The intention is to co-locate the styles with the react components in the upcoming refactor, you've just saved me a good bit of work.
3 participants
The Problem/Issue/Bug:
The Sass just needs a little bit better organization and a little refactoring of some of the variables to leverage Bootstrap 4's defaults.
How this PR Solves The Problem:
Begins to modularize code and leverages the Bootstrap 4 variables for colors throughout.
Manual Testing Instructions:
DDEV GUI should still appear identical to previous commit while implementing the new styles.
Related Issue Link(s)
#89