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

replace .visible-{size} with .visible-{size}-{display} #12204

Merged
merged 8 commits into from
Mar 10, 2014
Merged

replace .visible-{size} with .visible-{size}-{display} #12204

merged 8 commits into from
Mar 10, 2014

Conversation

cvrebert
Copy link
Collaborator

Adds .visible-{screen size}-block, .visible-{screen size}-inline, and .visible-{screen size}-inline-block classes (and also a corresponding trio for .visible-print). Also deprecates the .visible-{screen size} classes.

Proposed fix for #8869 pursuant to #9397. Includes docs updates.

There is most likely a way to refactor this to be less repetitive & more concise by using some more sophisticated Less features, but this should be okay for the time being.

/to @mdo for review.

@juthilo
Copy link
Collaborator

juthilo commented Jan 13, 2014

Perhaps make it more clear in the docs that you can now use any of these classes to toggle table elements?

@mdo
Copy link
Member

mdo commented Jan 13, 2014

This is really verbose. Why not use new mixins for just the inline and inline-block variations? How often will you see any table related element go from inline to table, or table to inline? Don't see those mixing that often.

@mdo
Copy link
Member

mdo commented Jan 14, 2014

I think I'd rather wait to do this until v3.2 given we've only a few days left for v3.1.

@cvrebert
Copy link
Collaborator Author

@mdo This has now been revised accordingly to exclude the table-related special casing.

@mdo
Copy link
Member

mdo commented Jan 15, 2014

Nice. Are we deprecating the generic classes then? Should we mention that in the docs and downplay that?

@cvrebert
Copy link
Collaborator Author

Are we deprecating the generic classes then?

Yes; at least, that's what I'm proposing.

Should we mention that in the docs

Yes, fair point. I'll further tweak the PR.

@cvrebert
Copy link
Collaborator Author

Okay, the deprecations are now mentioned in the docs.

@juthilo
Copy link
Collaborator

juthilo commented Jan 18, 2014

Don't the hidden-xs, -sm, -md, -lg, and -print classes need to be deprecated as well, as .responsive-invisibility() will be deprecated?

@cvrebert
Copy link
Collaborator Author

IMHO, no. We'll merely be able to simplify those classes to not be defined using the mixin in v4, but their semantics should be unaffected except possibly in some very obscure edge cases.

@juthilo
Copy link
Collaborator

juthilo commented Jan 18, 2014

I see and agree, thanks for explaining.

Is this perhaps suitable for inclusion in 3.1.0 after all? Seems it's all set except for a rebase? Don't wanna rush anything though... :)

@cvrebert
Copy link
Collaborator Author

Rebase complete :-)

@juthilo
Copy link
Collaborator

juthilo commented Jan 19, 2014

⭐⭐⭐⭐⭐ ;)

@cvrebert
Copy link
Collaborator Author

Okay, the unrelated fix that made the classes chainable also made my changes not merge anymore, but I've now pushed a revised version.

@mdo
Copy link
Member

mdo commented Feb 14, 2014

Word, I'll peep this again soon.

@mdo
Copy link
Member

mdo commented Feb 22, 2014

@cvrebert Is there something to update here regarding recent or conflicting changes?

@cvrebert
Copy link
Collaborator Author

@mdo No conflicts, but I've just rebased this again so that Travis will turn green. Just needs QA+review.

@mdo
Copy link
Member

mdo commented Feb 22, 2014

Thinking ahead more, I have two questions:

  • What about table displays? Tables, rows, cells, headers, etc. Where do we draw the line?
  • What about that other issue that asks for breakpoint and below classes? For example, hide on medium and below? Does this then balloon out if we have to support those classes as well?

@cvrebert
Copy link
Collaborator Author

IMHO FWIW:

  • "size X and above/below" seems more common and makes more sense than "at only size X".
  • having both "show" and "hide" classes seems unnecessary since they're symmetrical; only one kind is really needed
  • if possible, it'd be best to use a technique that's generic with respect to the non-hidden display value, so as to minimize the number of classes (WRT your point about table-related displays)

(This PR doesn't comply with all these goals.)

@alexislefebvre
Copy link

@cvrebert: I have a menu where I display normal texts on xs and lg screens, and short texts on sm and md screens. So I needed to use both show and hide classes.

@DaSchTour
Copy link
Contributor

can't the .visible-{screen size} be left as it is and only the -inline and -inline-block be added.
Normally it works as it should and I think many people wouldn't like to replace all their .visible and .hidden just for some who need and inline visible/hide.

@alexislefebvre
Copy link

What about adding a class for setting the display:inline-block;?

For example: <div class="visible-sm visible-inline-block">...</div>.

And the CSS:

@media (min-width: 768px) and (max-width: 991px) {
  /* current class */
  .visible-sm {
    display: block !important;
  }
  /* new class */
  .visible-sm.visible-inline-block {
    display: inline-block !important;
  }
...

I'm pretty sure no one will need to display a block as inline or inline-block in 2 different sizes. So the combination of the 2 classes should be sufficient.

With this class, backwards compatibility won't be broken.

@mdo
Copy link
Member

mdo commented Feb 25, 2014

@alexislefebvre We avoid chained classes in Bootstrap whenever possible.

mdo added a commit that referenced this pull request Mar 10, 2014
replace .visible-{size} with .visible-{size}-{display}
@mdo mdo merged commit e727973 into master Mar 10, 2014
@mdo mdo deleted the fix-8869 branch March 10, 2014 05:48
@mdo mdo mentioned this pull request Mar 10, 2014
@mdo
Copy link
Member

mdo commented Mar 10, 2014

💎💎💎💎💎💎💎💎💎💎🚢💎💎💎💎💎💎💎💎💎
💎💎💎💎💎💎💎💎💎🚢🚢🚢💎💎💎💎💎💎💎💎
💎💎💎💎💎💎💎💎🚢💎🚢💎🚢💎💎💎💎💎💎💎
💎💎💎💎💎💎💎🚢💎💎🚢💎💎🚢💎💎💎💎💎💎
💎💎💎💎💎💎🚢💎💎💎🚢💎💎💎🚢💎💎💎💎💎
💎💎💎💎💎🚢💎💎💎💎🚢💎💎💎💎🚢💎💎💎💎
💎💎💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎💎
💎🚢🚢💎💎💎💎💎💎💎🚢💎💎💎💎💎💎🚢🚢💎
💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎
💎💎💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎💎

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 10, 2014

After ASCII art now comes Emoji art; It's great, I love it!

@Shane32
Copy link

Shane32 commented Apr 25, 2014

I would like to point out another solution (below). It maintains existing syntax and works with any type of element, as it does not change the CSS display property except when hiding the element. However, it requires IE9+, so may only be useful when IE8 compatibility is dropped.

@media (max-width: 767px) {
  .visible-sm:not(.visible-xs),
  .visible-md:not(.visible-xs),
  .visible-lg:not(.visible-xs) {
    display: none !important;
  }
}
@media (min-width: 768px) and (max-width: 991px) {
  .visible-xs:not(.visible-sm),
  .visible-md:not(.visible-sm),
  .visible-lg:not(.visible-sm) {
    display: none !important;
  }
}
@media (min-width: 992px) and (max-width: 1199px) {
  .visible-xs:not(.visible-md),
  .visible-sm:not(.visible-md),
  .visible-lg:not(.visible-md) {
    display: none !important;
  }
}
@media (min-width: 1200px) {
  .visible-xs:not(.visible-lg),
  .visible-sm:not(.visible-lg),
  .visible-md:not(.visible-lg) {
    display: none !important;
  }
}

@twbs twbs locked and limited conversation to collaborators Apr 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants