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

how to override a mixin ?? #2532

Closed
argb opened this issue Apr 1, 2015 · 11 comments
Closed

how to override a mixin ?? #2532

argb opened this issue Apr 1, 2015 · 11 comments

Comments

@argb
Copy link

argb commented Apr 1, 2015

in bootstrap there is a mixin
// Responsive image
//
// Keep images from scaling beyond the width of their parents.

.img-responsive(@display: block) {
  display: @display;
  max-width: 100%; // Part 1: Set a maximum relative to the parent
  height: auto; // Part 2: Scale the height according to the width, otherwise you get stretching
}

note that the last statement-> height:auto ,This is really bad idea,I need to set a fixed size image attribute like <img width="300px" height="200px".../>,but you know it don't work, so i want to override the mixin to this:

.img-responsive(@display: block) {
  display: @display;
  max-width: 100%; // Part 1: Set a maximum relative to the parent
}

you know it don't work too,so bad...
bootstrap shouldn't set the style,and less should let me override it,is there any way to fix the problem?

@seven-phases-max
Copy link
Member

This is really bad idea,I need to set a fixed size image attribute

Then just set it. You do realize that setting a height: 200px after height: auto will override the latter, don't you?

Either way you can't override a mixin[1]. Just like you can't override a CSS class for example, but just like in CSS you can cascade it and override its properties.


[1] - unless the initial mixin is defined with default() guard or so, but this is another story and most likely never be the case in Bootstrap.

P.S. Also please use backticks to format your code.


P.P.S. Speaking of the use-case example, it's rather strange (to say at least), thought I guess Bootstrap folks will tell you the same there in more details:
"Responsive" image is an image that resizes to fit its container, so as soon as you set fixed width and/or height for the image and/or skip height: auto it is no longer responsive. I.e. now it turns out to be not so clear why you need to use .img-responsive at all for your use-case and most likely you're just misusing it (see for example).

@seven-phases-max
Copy link
Member

I'm pretty sure it should be a duplicate of this FR somewhere around though I can't find it right now (related but not the same: #1372).

@argb
Copy link
Author

argb commented Apr 1, 2015

height: auto can make thumbnail looks weird ,
media="all"

.img-responsive, .thumbnail>img, .thumbnail a>img, .carousel-inner>.item>img, .carousel-inner>.item>a>img {
  display: block;
  max-width: 100%;
  height: auto;
}

in this case that I have some images use the .thumbnail class,but images' size is different,I want to force set set same size to make sure the page not being mass,but this height:auto style stop me doing it,that's why I think it's wrong.

@seven-phases-max
Copy link
Member

Again: please use backticks to format your code.


but images' size is different,I want to force set set same size

I think this is more like a question for Bootstrap guys, but in short:
See my example above, you need to fix the size of the container (yes, you will need one container per a thumb (that's quite common pattern actually)) and not the size of the image itself.
And if you still need to set the image size directly for some reason you should not use BS .thumbnail class then, just write you own class(es) instead (e.g. my-fixed-size-thumbnail).
Yet, again if you remove height: auto from .img-responsive it just can't be named "responsive" anymore simply by definition.

@argb
Copy link
Author

argb commented Apr 1, 2015

Anyway,thanks for your kind reply. Goodnight!

@OrganicPanda
Copy link

It would be great to consider some syntax to make this possible, it seems completely backwards to me.

It makes working with Bootstrap (I would guess the most widely used instance of less) awkward when you need to make changes. If i want to change the padding of buttons by 2px (for example) my CSS gets out put as

.btn {
  /* ... */
  padding: 8px 24px;
  padding: 6px 24px;
  /* ... */
}

Which just seems weird, there is no benefit over simply adding .btn { padding: 6px 24px } to my css. I would have thought that less had my back here :(

@matthew-dean
Copy link
Member

@OrganicPanda
Even as a contributor and frequent user of Less, I often forget that overriding properties is not simple. More importantly, it's outside the scope of Less. Because of things like:

.row {
  display: table;  // non flexbox browsers
  display: flex;
}

Other tools like Pleeeease (or however many e's it has), or other such CSS shimming tools can determine which properties can be safely dropped / minified based on the a) values used and the b) browsers targeted, c) the current usage of those browsers. Tracking all of those components and how they apply to each property is not the goal of Less. Less is a tool for writing CSS, but it doesn't have an opinion on whether your CSS properties are valid or redundant in a particular browser.

@OrganicPanda
Copy link

I guess I wasn't clear about my issue. Bootstrap has variables and mixins, overriding variables works as you would expect but overriding mixins doesn't match the behaviour, which is disappointing. Bootstrap has a mixin to create various button sizes, I would like to change that behaviour to do something different but less instead applies their mixin and then mine which is not what I would expect.

Overriding mixins is not out of the scope of less because they are a core construct of less and not part of css, they should match how less already deals with variables rather than having two different approaches in the same language.

I hope that clarifies that I'm not looking for auto-deduping of properties - that is obviously a terrible idea :)

@seven-phases-max
Copy link
Member

@OrganicPanda

Bootstrap has a mixin to create various button sizes, I would like to change that behaviour to do something different but less instead applies their mixin and then mine which is not what I would expect.

It's not a problem to make a mixin overridable in Less by now (the easiest way is to add when default() to it for example. See #2211 for concrete examples). So it's more a question of a library design rather than of a dedicated Less feature.

@OrganicPanda
Copy link

I suppose from the language design point of view maybe you are right to not include it but from a users point of view that is a very disappointing answer that, in practise, makes it hard to work with the most ubiquitous library currently available.

Thanks for taking the time to answer anyway

@stale
Copy link

stale bot commented Nov 14, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 14, 2017
@stale stale bot closed this as completed Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@argb @OrganicPanda @matthew-dean @seven-phases-max and others