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

v4 clearfix on .card-{block,header,footer} breaks flex layouts #18849

Closed
ju1ius opened this issue Jan 11, 2016 · 12 comments
Closed

v4 clearfix on .card-{block,header,footer} breaks flex layouts #18849

ju1ius opened this issue Jan 11, 2016 · 12 comments

Comments

@ju1ius
Copy link
Contributor

ju1ius commented Jan 11, 2016

In latest v4-dev branch, the clearfix mixin is added on all the card subcomponents.
If the subcomponent is added a flex display, the :after pseudo added by the mixin adds unwanted spacing at the end of the flex block's main axis.

.card-footer {
  display: flex;
  justify-content: space-between;
  align-items: center;
}
<div class="card">
  <img class="card-img-top" src="http://loremflickr.com/320/180/space-oddity">
  <div class="card-block">
    <h2 class="card-title">Space Oddity</h2>
    <div class="card-subtitle">David Bowie ✝</div>
  </div>
  <div class="card-footer">
    <span>Release: 1969</span>
    <div class="btn-group">
      <div class="btn btn-sm btn-primary"><i class="fa fa-play"></i></div>
      <div class="btn btn-sm btn-warning"><i class="fa fa-star"></i></div>
    </div>
  </div>
</div>

I think the mixin should be guarded by a @if(not $enable-flex) block;

@mendeza
Copy link

mendeza commented Jan 11, 2016

I think you meant class="card-subtitle"

@ju1ius ju1ius changed the title clearfix on .card-{block,header,footer} breaks flex layouts v4 clearfix on .card-{block,header,footer} breaks flex layouts Jan 11, 2016
@cvrebert
Copy link
Collaborator

X-Ref: #18655

@cvrebert cvrebert added this to the v4.0.0-alpha.3 milestone Jan 16, 2016
@cvrebert
Copy link
Collaborator

@ju1ius I can't seem to reproduce any difference in spacing on either Chrome or Firefox on OS X between Flexbox and non-Flexbox cards. Are you using some other browser? Could you post a JS Bin or JS Fiddle that demonstrates the difference?
Flexbox example: https://jsfiddle.net/6ancmdx0/6/
Non-Flexbox example: https://jsfiddle.net/t9rnra28/3/

@ju1ius
Copy link
Contributor Author

ju1ius commented Jan 16, 2016

Here are the updated examples:
Without fix:
Flexbox: https://jsfiddle.net/6ancmdx0/7/
Non-Flexbox: https://jsfiddle.net/t9rnra28/5/
With fix:
Flexbox: https://jsfiddle.net/6ancmdx0/9/
Non Flexbox: https://jsfiddle.net/t9rnra28/6/

In all examples, I added

.card-footer {
  display: flex;
  justify-content: space-between;
}

Expected result: the first child of .card-footer should be aligned to the left of it's parent padding-box, and the last child to the right.

The issue is with the code in scss/_card.scss on line 83.

.card-footer {
  @include clearfix;
  // ...
}

It adds an ::after pseudo element with display: table to the .card-footer element.
If I then add display: flex to the .card-footer, the ::after pseudo becomes a flex-item, thus breaking the desired alignment.

That's why the code in the above-mentioned file should be...

.card-footer {
  @if (not $enable-flex) {
    @include clearfix;
  }
  // ...
}

...idem with .card-header, .card-block, and probably also in other places in the framework.

Because if I enable the flexbox option, I don't need to clear floats that don't exist ! 😉
And after a quick look at the card component, I can't see a reason for these clearfixes, since nothing is floated. Maybe they could be removed entirely ?

@ju1ius
Copy link
Contributor Author

ju1ius commented Jan 16, 2016

Thanks @twbs-lmvtfy, but this is irrelevant to the issue. 😇

@cvrebert
Copy link
Collaborator

And after a quick look at the card component, I can't see a reason for these clearfixes, since nothing is floated. Maybe they could be removed entirely ?

I believe they are needed because users might choose to include floated things in the card. See the first card in my earlier examples, and #18655.

@cvrebert
Copy link
Collaborator

In all examples, I added

Ah, I hadn't been clear on the fact that custom CSS was needed to trigger the problem.

@cvrebert
Copy link
Collaborator

Hmm, but even with Flexbox enabled, the Card itself is still display:block by default.
This is above my CSS paygrade. Gonna leave this one to @mdo.

@ju1ius
Copy link
Contributor Author

ju1ius commented Jan 16, 2016

I believe they are needed because users might choose to include floated things in the card. See the first card in my earlier examples, and #18655.

I'm also suspecting a «clearfix all the things just in case» syndrom.
In this case I think the mixins calls should be entirely removed since:

  1. People usually know why, where & when to clear floats, and there's an utility class for that.
  2. It interferes with display: flex on mentioned card subcomponents

#18633 and #18655 should be closed as invalid.

@cvrebert
Copy link
Collaborator

Apparently the plan is to Flexbox-ify all the things (#18901 (comment)), so #18851 sounds reasonable, FWIW.

@thierryk
Copy link

thierryk commented May 28, 2016

And after a quick look at the card component, I can't see a reason for these clearfixes, since nothing is floated. Maybe they could be removed entirely ?

It does not really matter if something is floated in there. In the examples on jsfiddle.net, the button styled via pull-xs-right is floated to the right (float:right !important) but shows on the left side of the box. This is because its computed value is float:none. In a "flex" context, float has no effect, hence there is no need for clearfix.

@twbs-lmvtfy
Copy link

Hi @thierryk!

You appear to have posted a live example (https://fiddle.jshell.net/6ancmdx0/7/show/light/), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:

  • line 6843, column 7: E047: .btn should only be used on <a>, <button>, <input>, or <label> elements.
  • line 6844, column 7: E047: .btn should only be used on <a>, <button>, <input>, or <label> elements.
  • line 6821, column 5: W007: Found one or more <button>s missing a type attribute.
  • line 6826, column 5: W007: Found one or more <button>s missing a type attribute.
  • line 6829, column 5: W007: Found one or more <button>s missing a type attribute.

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

@mdo mdo modified the milestones: v4.0.0-alpha.3, v4.0.0-alpha.4 Jul 23, 2016
@mdo mdo modified the milestones: v4.0.0-alpha.5, v4.0.0-alpha.6 Oct 19, 2016
@mdo mdo closed this as completed in bd62b18 Oct 28, 2016
mdo added a commit that referenced this issue Oct 28, 2016
Don't clearfix if flexbox is enabled. Fixes #18849
@mdo mdo added the has-pr label Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Alpha 6
Fixed/Merged
Development

No branches or pull requests

6 participants