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

make visible/hidden classes mixable #9821

Merged
merged 3 commits into from
Aug 19, 2013
Merged

make visible/hidden classes mixable #9821

merged 3 commits into from
Aug 19, 2013

Conversation

eratzlaff
Copy link
Contributor

Change on responsive-utilities for hidden and visible to be mixable
example:
class="visible-xs visible-sm"
test on mixins
example:

.hide-xs-sm {
  .hidden-xs;
  .hidden-sm;
}

@masterbee
Copy link
Contributor

@eratzlaff - I noticed that you are using some older LESS variables for the media queries. I believe that is what if falling in your build. Here are the new variables if you want to tweak your code.

// Extra small screen / phone
@screen-xs:                  480px;
@screen-phone:               @screen-xs;

// Small screen / tablet
@screen-sm:                  768px;
@screen-tablet:              @screen-sm;

// Medium screen / desktop
@screen-md:                  992px;
@screen-desktop:             @screen-md;

// Large screen / wide desktop
@screen-lg:                  1200px;
@screen-lg-desktop:          @screen-lg;

// So media queries don't overlap when required, provide a maximum
@screen-xs-max:              (@screen-sm - 1);
@screen-sm-max:              (@screen-md - 1);
@screen-md-max:              (@screen-lg - 1);

@mdo amd @eratzlaff - I just want to mention that @eratzlaff method does work well, but it does introduce a chunk more class relationships, which in turn hike up the CSS a bit. I know that @mdo is being a bit frugal on CSS size, so just wanted to mention. The solution proposed by @turchenkoalex in pull #9692, is leaner and I am setting sometime aside later this week to do some tests, but this does the trick as well.

@eratzlaff
Copy link
Contributor Author

@masterbee fix variable name.
I'm agree of the size of this solution. But the solution of @turchenkoalex is leaner, but don't work with mixins like I show above.
I mix my solution with @turchenkoalex solution on a comment.

@mdo
Copy link
Member

mdo commented Aug 19, 2013

We won't rush into this on the eve of v3, though I appreciate the rapid response <3.

Let's see what other solutions are because the chaining will also be an issue with performance for larger scale projects and pages. Let's let this bake, build some additional docs to showcase the combination of classes, and really test this out.

@mdo
Copy link
Member

mdo commented Aug 19, 2013

@masterbee @turchenkoalex And yes, we definitely need these to be fully mixin-able, whatever the solution we end up with.

@eratzlaff
Copy link
Contributor Author

#9692 (comment) on this comment I put my test @mdo
I see if I find a workaround without chaining.
But I think this issue must be fixed before v3.
Maybe use this solution and open a new issue related to performance on this fix. If whee don't get a solution without chaining.

@mdo
Copy link
Member

mdo commented Aug 19, 2013

I don't believe v2 even supported this, so I'm not inclined to add it in a rush. It doesn't need to absolutely be in there for the initial launch. This is something we can improve upon :).

@eratzlaff
Copy link
Contributor Author

v2 don't need this because you only have 3 grid size, now the are 4.
on v2 if you need something like hidden-xs hidden-sm I could replace it with visible-lg

@turchenkoalex
Copy link

Maybe it not necessary to have mixable classes for all visible/hidden classes in v3. I think that main case for use of this feature is setting visible/hidden for mobile(xs, sm) and non mobile devices (md, lg). What is your @mdo, @eratzlaff opinion about this?

@mdo
Copy link
Member

mdo commented Aug 19, 2013

Alright, screw it. Let's see what we can do. Where are your tests for this @eratzlaff?

@eratzlaff
Copy link
Contributor Author

Test on comment #9692 (comment)
the second and last block are the test I make.
One are a combination of mixin.
2 size show-xs-sm and 3 size show-xs-sm-md
and the same for hide-
On the html code I make two column which should look example the same on all size
The first column for 'visible-xs visible-sm' and the second column for show-xs-sm for example.
4 simple combination
6 combination of 2
and 4 combination of 3 classes.

@mdo
Copy link
Member

mdo commented Aug 19, 2013

You'll need to do better than those I'm afraid—it's super difficult to see each one change.

Can you setup something like the test cases under http://getbootstrap.com/css/#responsive-utilities?

@turchenkoalex
Copy link

@eratzlaff Combination of 3 classes it is a inverse of class. Such as show-xs-sm-md it hidden-lg.

@eratzlaff
Copy link
Contributor Author

@turchenkoalex I know, Its only to complete the test on all possible combination.
@mdo 👍 I make It like the documentation.

@mdo mdo merged commit 7818c5e into twbs:3.0.0-wip Aug 19, 2013
@mdo
Copy link
Member

mdo commented Aug 19, 2013

Merged! Had to fix your examples, but no problem there. Also updated the display, subnav links, and more. Thanks for pushing on this so quickly :).

@turchenkoalex
Copy link

@eratzlaff, @mdo thanks a lot. It very helpfull feature!

@eratzlaff
Copy link
Contributor Author

:) you're welcome 👍

@mdo
Copy link
Member

mdo commented Aug 19, 2013

<3<3<3

@dstroot
Copy link

dstroot commented Aug 19, 2013

Nice!

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

Successfully merging this pull request may close these issues.

5 participants