-
-
Notifications
You must be signed in to change notification settings - Fork 997
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
Less variables #70
Comments
Did you tested with 2.0's variables? |
I fount where is my mistake, generator use require twitter/bootstrap in application.js and not bootstrap, so this file has never been readed by asset pipeline. But @gridColumns variable didn't worked for me. |
Some changes and fixes applied, can you try again with updating gem. |
I updated and used this in bootstrap_override:
|
I can confirm that, with the latest git master of this gem, overriding Bootstrap's less variables is extremely not obvious. the installed application.css looks like this:
Sprockets processes each require individually, and it's (theoretically) satisfied by any type of preprocessor and could be in any language. There's no variable sharing or anything like that among them. Twitter bootstrap (vanilla, from vendor) is processed, with its default variables, in lines 2 and 3 above. Line 4 will require the whole tree, which should pull in bootstrap_override.css.less, which looks like this:
Unfortunately, even if you set the right variables there, it's too little too late - it'll only be consumed by bootstrap_base (which internally only has variables and mixins). This is almost a NO-OP since the actual bootstrap widgets were not rebuilt, and are served verbatim with their default variables earlier. What raphaelcosta did above is going doen the right path - you want the same LESS file to require both the whole twitter/bootstrap framework AND your custom variables. Unfortunately, without further modifications, that means that the whole packege will actually load twitter/bootstrap twice - once directly from application.css using the default variables, and again from within bootstrap_override.css.less using the custom variables - both chunks will be in the output, and its only by virtue of same CSS rules overriding ealier ones that the browser picks up on the later ones. I think, going forward, application.css should NOT require twitter/bootstrap, since this leaves 0 room for overrides. Instead it should always just require_tree or explicitly require bootstrap_overrides (to be renamed bootstrap_and_overrides for clarity). Then, that file, which is a single LESS invocation from sprockets, should require the full twitter/bootstrap and allow for overrides there. |
I should have documented my patches better. The idea behind that bootstrap_override is just to give you variables and mixins you may use to re-style elements. The defaults can't be override atm. There are efforts to bring that feature in, but the approach is far from being robust. I actually came up with something that may work, will post back later when I get home. |
With my suggestion above:
All less default variables are nicely overrideable, AND you get to inherit mixins for styling your own elements (though the purpose of bootstrap_override changes in that case, that's why I recommend changing it to bootstrap_and_overrides) |
@minaguib Can you please fork and issue a pull to me with your suggestions? I'll test it. If everything overrideable I'll merge with master. |
Sure thing, but I'd like to hear from @datl some more - since this change will revert some of his work. |
Sounds good, just go ahead and submit your pull request please. |
@datl I thought about your comment above, but I implemented what I think is a simpler solution. application.css always requires bootstrap_and_overrides - the generator/installer sets it up that way, end of story. We already rely on Less to compile bootstrap - we're just moving that invocation 1 file away. For both types of users, it "just works". For users who actually want to tinker in Less-land, they can open that file and add their changes there. This can be done after-the-fact, and the install process remains the same for everyone. I don't think it's worth worrying about "what if the user deletes 'require bootstrap_and_overrides'" from application.css any more than worrying about them deleting 'require jquery' - they're responsible for managing their manifest, and the installer sets them up on the right track initially. |
@minaguib .Your pull merged, thanks. New version of gem will be released soon. |
@minaguib Agree w/ you that it would be more straightforward to keep the installation process the same to everyone. So instead of giving one more install option, I would suggest having a comment block in the less file saying that they can delete that file and include bootstrap in application.css if they dont wish to override stuff. And one problem with the pull request, you are compiling both bootstrap and bootstrap_responsive in the same less file, which brings back issue #87. We would want to have those compiled separately. |
@datl I'm not familiar enough with the responsive stuff, but, can we solve that by adding a comment in bootstrap_and_overrides ? For example:
|
Nope. I believe we need bootstrap in order to have bootstrap_responsive fully functioned. Also, many people are gonna need both of those in their projects anyway, because responsive stylesheet is only for devices with small screen. We can either put ''@import twitter/bootstrap/responsive'' into a separated less file or delete variables and mixins import lines in bootstrap/responsive.less. |
Ok I think I completely understand the responsive behavior and chain now. I think this would be the ideal scenario: application.css:
bootstrap.css.less:
bootstrap_responsive.css.less:
bootstrap_overrides.css.less:
Unfortunately it's taking half a step back (in the right direction, IMO), and will cause a bit more confusion to existing users of the gem, who are already struggling a bit with the change we pushed above... :( To address your solutions above:
Thoughts everyone ? |
This is already implemented in verson 2.0.1.0 ? |
I'm a bit confused about this issue. @import "twitter/bootstrap/bootstrap";
@import "twitter/bootstrap/responsive";
@iconSpritePath: asset-path('twitter/bootstrap/glyphicons-halflings.png');
@iconWhiteSpritePath: asset-path('twitter/bootstrap/glyphicons-halflings-white.png');
@linkColor: #ff0000;
@gridColumns: 12;
@gridColumnWidth: 80px;
@gridGutterWidth: 0px;
@gridRowWidth: (@gridColumns * @gridColumnWidth) + (@gridGutterWidth * (@gridColumns - 1)); This is what I have in my The only variable in the above snippet that properly overrides is the I'm using the latest commit from the git repository. Is there any way to alter the Thanks |
@meskyanichi I'll experiment and get back to you |
I noticed that when I comment out the |
I have the same problem as @meskyanichi and the workaround of commenting out |
this workaround worked for me too, but not in all variables, for example gridColumns didn't worked was I expected |
that is happens because the twitter/bootstrap/responsive load the default variables of bootstrap. If you have a look in native twiter bootstrap code at less / responsive.less (https://github.com/twitter/bootstrap/blob/master/less/responsive.less) file you will notice that at line 21 it loads the variabes every time |
Searching more on this I realize that the error is in |
@daniely i need respective, how could i walk around if i don't not commenting out @iotriado, bootstrap also load the variabels (https://github.com/twitter/bootstrap/blob/master/less/bootstrap.less#L15), but why only responsive have bugs? @seyhunak need your help, thanks. |
I had an undefined variable error. Restarting the server didn't fix it but |
I am using master branch version and i am trying to override some less variables in the bootstrap.css.less but it is not working.
@import "twitter/bootstrap";
// Baseline grid
@basefont: 20px;
@baseline: 18px;
@gridColumns: 24;
@gridGutterWidth: 20px;
@gridColumnWidth: 21px;
@primaryButtonBackground: white;
The text was updated successfully, but these errors were encountered: