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

Add mixin make-container() similar to make-row() #10160

Closed
wants to merge 8 commits into from

Conversation

whazap
Copy link

@whazap whazap commented Aug 26, 2013

When using the mixins make-row() & make-column() instead of using .row & .col- classes, i was missing the same for creating a page container.

padding-right: (@gutter / 2);
padding-left: (@gutter / 2);

@media (min-width: @screen-sm) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@screen-sm etc. have been deprecated; see the comments near their declarations in the git HEAD version.

@ggam
Copy link
Contributor

ggam commented Aug 29, 2013

I think your mixin is a bit rigid. Calling make-container will create containers only for the core default breakpoints, but what if someone adds custom breakpoints? Some people are already asking for another breakpoint at 480px on #10203.

I'd prefer something like, ad advertise the user that he has to manually call it for every breakpoint

.make-container(@gutter: @grid-gutter-width; @min-width; @max-width) {
  .clearfix();
  margin-right: auto;
  margin-left: auto;
  padding-right: (@gutter / 2);
  padding-left: (@gutter / 2);

  @media (min-width: @min-width) {
    max-width: @max-width;
  }
}

Also, as BS3 is mobile-first, fluid container should be the default. So I propose to rename your make-container to make-container-fixed and make-container-fluid to make-container.

PS: don't forget to remove container-fixed at https://github.com/whazap/bootstrap/blob/82be6299b2cd4f91a77ad6f11fef8d6d88805ad6/less/mixins.less#L545

@whazap
Copy link
Author

whazap commented Aug 30, 2013

great input, thanks, i will change the names of the mixins remove .container-fixed and do something similar to your mixin

@@ -538,16 +538,45 @@
td& { display: none !important; }
}

.mq-min-width(@min-width, @property, @value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to be using this mixin anywhere. I propose you to remove it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know how that one slipped in :) will do

@ggam
Copy link
Contributor

ggam commented Aug 30, 2013

I've made some more comments. Sorry for posting/deleting so much times :( hehe.

@dminkovsky
Copy link

Something like this would be pretty clutch for me too. Right now there's no way to use container as a mixin, that I'm aware of, with basic LESS. I even posted to StackOverflow but have had no luck.

What's important for me is seeing this solution incorporate the .container @media queries . As I read the patch proposed here, that doesn't seem to be the case.

@whazap
Copy link
Author

whazap commented Sep 2, 2013

@dminkovsky it does what you are asking for.

#element {
  .make-container-fixed();
}
or
#element {
  .container;
}

@dminkovsky
Copy link

@whazap: yes, you are right, this PR does let me use .make-container-fixed() or .container.

I proposed a simpler PR over at #10483 that would be a stepping-stone to this one. My goal is to be able to use .container as a mixin.

@whazap whazap closed this Nov 9, 2013
@dminkovsky
Copy link

Why'd you close this PR?

@ghost
Copy link

ghost commented Nov 9, 2013

Ez létrehoz egy csomó duplikált kódot hívott make-konténer minden töréspont kívülről sajátos média ?

@ghost
Copy link

ghost commented Nov 9, 2013

+. Make-tartály fix egyedi (@ képernyő sm-min, @ konténer-tabletta, @ ereszcsatorna);Minden töréspont Kívülről sajátos média?

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

Successfully merging this pull request may close these issues.

4 participants