Move variables and Bootstrap lines to autoload #1993
Conversation
|
need to address we could add the variables import to the top of all the other autoload files in sage-installer, but that doesn't work for the 'none' option ..although we could add the autoloader w/ the variables import to the none option ¯_(ツ)_/¯ any ideas? |
resources/assets/styles/main.scss
Outdated
| @@ -1,7 +1,3 @@ | |||
| @import "~bootstrap/scss/functions"; | |||
| @import "common/variables"; | |||
There was a problem hiding this comment.
Actually, this line can be kept in the main.scss file.
If we remove it, other frameworks variables are not loaded, and ~bootstrap/scss/functions doesn't seem to override variables.
There was a problem hiding this comment.
i think the concern is using a bootstrap function in your variables
There was a problem hiding this comment.
Bootstrap themselves use functions in variables so it's confusing if we can't do the same in Sage.
It would either require repeated explanation on the forum, or a comment somewhere explaining why the following Bootstrap's own example for variable overrides doesn't work.
There was a problem hiding this comment.
You can always take the example from zurb/foundation, calling functions.scss inside variables.scss:
There was a problem hiding this comment.
You mean here in sage-installer?
That would work. Is that less messy than including common/variables.scss in each autoload? Is it more or less flexible in the future?
There was a problem hiding this comment.
Yes, yes, yes. 😆 👍
At least, it should be, imo.
There was a problem hiding this comment.
I'm comfortable with that solution. @retlehs do you have an opinion on including the BS functions right in the variables.scss file?
And forgive my ignorance, should I do that on this PR or is that a sage-installer only thing?
There was a problem hiding this comment.
sounds good to me, you'll want to do that both on this repo (as it should function properly w/ bootstrap without the installer) and sage-installer
|
I guess nobody can say we didn't thoroughly discuss the solution this time! I'll take a stab at sage-installer tomorrow unless somebody else gets there first <--hint ;) |
|
updated sage-installer + new version tagged, should be all good now. thanks @MWDelaney & @LeoColomb! |
See discussion on #1992