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 simple same-height columns to the grid with flexbox #11851

Closed
wants to merge 4 commits into from

Conversation

pete-otaqui
Copy link

Added very simple same-height columns support using the flexbox layout module.

This supports the standard xs, sm, md, lg breakpoints and requires the .row and .cols to have an additional class, like so:

<div class="row row-md-flex">
  <div class="col-md-6 col-md-flex">Short</div>
  <div class="col-md-6 col-md-flex">Tall<br />Tall<br />Tall<br />Tall<br /></div>
</div>

The current W3C CR for flexbox is supported by all modern browsers according to caniuse.com/flexbox, with some vendor prefixing for mobile browsers (Firefox and IE) and Safari (desktop and mobile).

I've added an example to the grid/index.html file, although this needs to be made nicer. I also didn't really have time to understand the best way to use the current less mixins to generate the classes correctly, so I think that could be improved as well.

I'd love to see same-height columns in BS, and I hope that this PR can at the very least start a discussion about it.

@pete-otaqui
Copy link
Author

It also occurs to me now that "-flex" is possibly not the right name to use in the classes, since it describes an internal property rather than the perceived effect.

I know this isn't right either, but maybe something more descriptive along the lines of .row-sm-sameheight and .col-sm-sameheight would be better.

@cvrebert
Copy link
Collaborator

We aim to support IE8+; flexbox support was added in IE10...
At the least, there'd need to be a compatibility warning in the docs.

@pete-otaqui
Copy link
Author

Sounds like a good idea :)

@boulox
Copy link
Contributor

boulox commented Dec 12, 2013

Regarding Marketshare of ie, around 15% to 30% (depend of source :/ ) of users will have a broken experience on desktop...

@pete-otaqui
Copy link
Author

Yes absolutely.

I'm not sure of the BS policy with progressive enhancement, but this obviously doesn't really "break" anywhere (or at least not in the way I would use the term!) it just has zero effect.

@mdo
Copy link
Member

mdo commented Dec 12, 2013

I'm not sure of the BS policy with progressive enhancement, but this obviously doesn't really "break" anywhere (or at least not in the way I would use the term!) it just has zero effect.

I'll allow it, with proper information in the docs.

.displayFlex();
}
}
@media (min-width: @screen-md) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one should be lg?

@pete-otaqui
Copy link
Author

Correct.

I've fixed those things already, and some other pieces too (like removing the actually unnecessary col-*-flex classes).

I'm working on some docs for this, but for some reason I can't get the TOC in the jekyll-generated output to update with the new section.

@cvrebert
Copy link
Collaborator

You're aware that the TOC isn't autogenerated, right?

@pete-otaqui
Copy link
Author

I figured that was it but couldn't see it in the code :S

@pete-otaqui
Copy link
Author

I've fixed the errors you pointed out, removed some unnecessary code, tidied up the example and added the docs (I found the _includes/nav-css.html file in the end!)

I added in the classname of the row in the docs, which isn't done anywhere else, like this:

<div class="col-md-4">.row-md-flex &gt; .col-md-4</div>

I wasn't sure if that was the correct thing to do.

@pete-otaqui
Copy link
Author

Hopefully this is good to go, but if you have any feedback on the latest code & documentation I'd love to hear it.

NB - this works in IE 10+, so the non-functional percentage of users will be higher. A bit like the rounded corners on buttons really :)

@carasmo
Copy link

carasmo commented Dec 13, 2013

Rocks!

@pete-otaqui
Copy link
Author

For anyone coming across this thread wanting to implement this yourself, you can do so with this css (nb, I've skipped the full definitions for all the declarations for brevity):

.row-xs-flex {
  display: flex;
  display: -webkit-flex;
  display: -moz-flex;
  display: -ms-flex;
}
@media (min-width: 768px) {
  .row-sm-flex {
    display: flex;
  }
}
@media (min-width: 992px) {
  .row-md-flex {
    display: flex;
  }
}
@media (min-width: 1200px) {
  .row-lg-flex {
    display: flex;
  }
}

And this HTML:

<div class="container">
  <div class="row row-md-flex">
    <div class="col-md-6">short</div>
    <div class="col-md-6">tall<br/>tall<br/></div>
  </div>
</div>

<h3 id="grid-same-height-columns">Same height columns</h3>
<p>Give your grid columns the same heights using responsive classes on the row, such as <code>.row-md-flex</code>.</p>

<p>Note that this is not supported in Internet Explorer prior to version 10. In any unsupported browser, the <code>row-*-flex</code> classes will have no effect.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably be made a callout box

@zlatanvasovic
Copy link
Contributor

Wrong property order. Vendors first, classic last.

/* Bad */
.class {
  box-sizing: border-box;
  -moz-box-sizing: border-box;
}

/* Good */
.class {
  -moz-box-sizing: border-box;
  box-sizing: border-box;
}

@mdo
Copy link
Member

mdo commented Dec 19, 2013

Looking at this again, and given the browser support right now, I think this might be better served as an experimental example template (like offcanvas). When we get further along we can consider making it part of the main docs and source code (likely in v4).

You cool to update and rebase this pull request to do that @pete-otaqui? If not, that's chill, I can take a look at it for a later release.

@pete-otaqui
Copy link
Author

Cool, will do.

On 19 December 2013 03:58, Mark Otto notifications@github.com wrote:

Looking at this again, and given the browser support right now, I think
this might be better served as an experimental example template (like
offcanvas). When we get further along we can consider making it part of the
main docs and source code (likely in v4).

You cool to update and rebase this pull request to do that @pete-otaquihttps://github.com/pete-otaqui?
If not, that's chill, I can take a look at it for a later release.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11851#issuecomment-30896696
.

Pete Otaqui
pete@otaqui.com
+442081335086
+962779527337

@cvrebert
Copy link
Collaborator

cvrebert commented Jan 9, 2014

@pete-otaqui Just FYI, this will get punted to v3.2.0 if this PR isn't updated or a new PR isn't opened in the next several days.

@mdo
Copy link
Member

mdo commented Jan 10, 2014

@cvrebert @pete-otaqui Punting now. Too close to shipping to add another example.

@mdo
Copy link
Member

mdo commented Mar 7, 2014

I'm punting on this since I don't really want to work on this 😆. If someone wants to add the example, I'd welcome a new PR <3.

@Quix0r
Copy link

Quix0r commented Apr 18, 2018

The other issue #9939 links here. I want to mention that @brianmhunt 's solution at #9939 (comment) did actually work but needs a !important being added. Then still .btn-group-vertical remains as before and the button group is not wrapping (see issue starter's screenshot).

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.

8 participants