Skip to content
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

Added support for default variables #1104

Closed
wants to merge 1 commit into from

Conversation

mrcljx
Copy link
Contributor

@mrcljx mrcljx commented Jan 6, 2013

You can now define default variables via @var: 1px !default;. It will
behave as @var: 1px; if @var hasnt't been defined yet. Otherwise
it will be ignored.

This is useful for frameworks (e.g. Bootstrap) which need to define
variables to work but want to allow the user of the framework to
change them without messing with the internal less-stylesheets.

Sass has this feature and it is used in the SASS-fork of Bootstrap and comes in quite handy.

This pull-request is based on #313 but with a test and the changes moved to the right places.

You can now define default variables via `@var: 1px !default;`. It will
behave as `@var: 1px;` if `@var` hasnt't been defined yet. Otherwise
it will be ignored.

This is useful for frameworks (e.g. Bootstrap) which need to define
variables to work but want to allow the user of the framework to
change them without messing with the internal less-stylesheets.
@lukeapage
Copy link
Member

Nice to see you back, your unit and media bubbling work is great.

at the moment people have to put a variables file last and those definitions override earlier ones.. this would require bootstrap to put !default after every variable definition and would mean people could override a variable by putting it first..

could you go into the benefits and the scenario that requires this more?

@mrcljx
Copy link
Contributor Author

mrcljx commented Jan 7, 2013

Hi Luke,

sorry for not making it clearer in the original pull-request (wrote it just before going to bed). @Warry did a better job in #313. So another try:

I wrote a small CSS-framework for some of my private/freelance projects. I'm using Bootstrap as an example because it is public and has a similar (near identical) structure to mine and I think it also could benefit from the !default feature (although adopting it would be up to @mdo and @fat).

When you do @import "bootstrap" Bootstrap imports all of its sub-files like a bootloader - amongst them: variables.less, the file that defines all variables.

Now for example you want to change @baseFontSize to something other than its default 14px.

Add variable after bootstrap

@import "bootstrap";
@baseFontSize: 20px;

Cons: Doesn't affect bootstrap.

Do it "the way it's supposed to be done"

  • Download bootstrap less files into your projects bootstrap folder.
  • Change variables in variables.less

Cons: This makes updating to a newer Bootstrap difficult. You need to extract your local changes of variables.less and apply it to the newer one. Essentially you get the I forked Bootstrap and now I have to merge experience. Nothing a true Git user couldn't handle but less.js and Bootstrap have reached far beyond that user group.

Define your own my-bootstrap.less which is identical to bootstrap.less

except... two options:

  1. Import my-variables.less instead of variables.less, where my-variables.less is a copy of
  2. Import my-overrides.less after variables.less.

Option 1 Cons: Has the same issues as before.
Option 2 Cons: Has a caveat: variables.less has variables that depend on each other (as seen below). Thus @fontSizeLarge would still be ~18px instead of 25px;

@fontSizeLarge:         @baseFontSize * 1.25; // ~18px

New approach - assuming Bootstrap adopts !default

@baseFontSize: 20px;
@import "bootstrap";

Pros: Updating to newer Bootstrap versions would be totally easy (unless they change their "API", i.e. their variable names).
Cons: Requires this pull-request to be pulled.


I hope this is a better illustration of the scenario and makes it easier to decide whether or not to pull.

Anyway, it wouldn't bother me if this pull request is rejected since it adds a complete new syntax and I'm not convinced that this isn't a niche interest.

@lukeapage
Copy link
Member

I'm just surprised the "Add variable after bootstrap" case doesn't work. I wonder if its because the import is evaluated before being mixed into the main less file and getting the overriding variable

@mrcljx
Copy link
Contributor Author

mrcljx commented Jan 7, 2013

Now I need an emoticon for a facepalm. Seems to be an issue of the node.js Framework I am using... just tested it with a clean less and of course it worked.

@mrcljx mrcljx closed this Jan 7, 2013
@mrcljx
Copy link
Contributor Author

mrcljx commented Jan 7, 2013

Sorry again for wasting my time and yours. I hope to get back at less.js with a quality pull-request next time. ;)

@lukeapage
Copy link
Member

that is pretty strange if it's the node.js framework.. It could be a timing issue or a bug..

It took some debate to agree that the last definition should be used, not the last before the current rule's definition.. and there are some up and down sides to it. If it is changed then this would definitely be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants