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

Fix .clearfix on .dl-horizontal dd (#12753) #12756

Closed
wants to merge 2 commits into from

Conversation

dairiki
Copy link

@dairiki dairiki commented Feb 16, 2014

See #12753 for details.

Less' :extend, when used inside a @media declaration, only matches
selectors inside the same @media declaration.  Because of this,
`.dl-horizontal dd` was not getting the `.clearfix` styles.
@cvrebert
Copy link
Collaborator

Filed an upstream less.js bug regarding the duplication: less/less.js#1886

@cvrebert
Copy link
Collaborator

@mdo We're gonna need some kind of workaround here to avoid duplication.

@dairiki
Copy link
Author

dairiki commented Feb 18, 2014

Omitting the parenthesis on the declaration of the .clearfix mixin and dropping the then-redundant .clearfix { .clearfix(); } from utilities.less seems to work. I.e.

.clearfix {
  &:before,
  &:after {
    content: " "; // 1
    display: table; // 2
  }
  &:after {
    clear: both;
  }
}

.goober {
  .clearfix();
}

appears to behave as desired.

(On the other hand, the duplicate rules aren't the end of the world.)

@mdo
Copy link
Member

mdo commented Feb 22, 2014

Something like this should be fine I think:

// Horizontal description lists
//
// Defaults to being stacked without any of the below styles applied, until the
// grid breakpoint is reached (default of ~768px).

.dl-horizontal {
  dd {
    &:extend(.clearfix all); // Clear the floated `dt` if an empty `dd` is present
  }

  @media (min-width: @grid-float-breakpoint) {
    dt {
      float: left;
      width: (@component-offset-horizontal - 20);
      clear: left;
      text-align: right;
      .text-overflow();
    }
    dd {
      margin-left: @component-offset-horizontal;
    }
  }
}

We don't need to change the way we clearfix, just where the clearfix is. This shouldn't change much really since it's just a clearfix.

@dairiki
Copy link
Author

dairiki commented Feb 22, 2014

Yes, I suspect that's fine, too.

Do you want me to update this PR accordingly? (I.e. are you going to merge it if I do?)

@mdo mdo closed this in b99be29 Feb 22, 2014
@mdo mdo mentioned this pull request Feb 22, 2014
@dairiki
Copy link
Author

dairiki commented Feb 22, 2014

Thanks!

@mdo
Copy link
Member

mdo commented Feb 22, 2014

<3

stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
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.

3 participants