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

input-size is generating different CSS than I would expect #15074

Closed
caleb opened this Issue Nov 11, 2014 · 4 comments

Comments

Projects
None yet
5 participants
@caleb
Copy link
Contributor

caleb commented Nov 11, 2014

I'm using bootstrap-sass and in that version control sizing doesn't work (e.g. form-group-sm or form-group-lg etc, see twbs/bootstrap-sass#763)

Looking into what is causing this led me to the Less version of bootstrap, and I think something is wrong with the input-size mixin and how it's being used. While the Sass version is broken, and I think the Less version works, I believe the Less version isn't generating the intended CSS.

The less code from forms.less (https://github.com/twbs/bootstrap/blob/master/less/forms.less#L315):

.input-lg,
.form-group-lg .form-control {
  .input-size(@input-height-large; @padding-large-vertical; @padding-large-horizontal; @font-size-large; @line-height-large; @input-border-radius-large);
}

Generates this CSS code:

textarea.input-sm,
textarea.form-group-sm .form-control,
select[multiple].input-sm,
select[multiple].form-group-sm .form-control {
  height: auto;
}
…

That doesn't look like the intended CSS. I would expect this:

textarea.input-sm,
.form-group-sm textarea.form-control,
select[multiple].input-sm,
.form-group-sm select[multiple].form-control {
  height: auto;
}
…

Is this wrong? I would be interested in trying to create a patch if it is.

-Caleb

@hnrch02 hnrch02 added the css label Nov 11, 2014

@caleb caleb changed the title Form Control Sizing not what I would expect input-size is generating different CSS than I would expect Nov 11, 2014

@caleb

This comment has been minimized.

Copy link
Contributor Author

caleb commented Nov 14, 2014

This jsbin shows the bug: http://jsbin.com/nomobegivu/1/edit?html,css,output

The textarea has rows=20 set, but the output has a set height because the height: auto rule listed above isn't being matched. Remove the form-group-lg class and the text area goes back to its correct height.

@mdo mdo added this to the v3.3.2 milestone Nov 30, 2014

@mdo mdo closed this in 7c71b48 Nov 30, 2014

glebm added a commit to twbs/bootstrap-sass that referenced this issue Nov 30, 2014

@mmiller678

This comment has been minimized.

Copy link

mmiller678 commented Dec 3, 2014

I don't think the LESS fix works, but I could be mistaken. The resulting CSS I get is:
select.form-group-sm .form-control {..}
textarea.form-group-sm .form-control,
select[multiple].form-group-sm.form-control {..}

I expected:
.form-group-sm select.form-control {..}
.form-group-sm textarea.form-control,
.form-group-sm select[multiple].form-control {..}

The issue seems to be in the mixin as its pre-pending the selector. The selector is pre-pended even if the classes are nested. See this LESS example here:
http://lesscss.org/features/#parent-selectors-feature-changing-selector-order

Not sure of the fix as I played with it a bit, but by no means am I a LESS expert.

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Dec 3, 2014

Agreed, the nonsensical selectors are still present after the "fix":
7c71b48#diff-6972cbb37f0ec48771217d4915ea7e6fR2553
CC: @mdo

@cvrebert cvrebert reopened this Dec 3, 2014

@mmiller678

This comment has been minimized.

Copy link

mmiller678 commented Dec 3, 2014

This is how I got it to work without changing the mixin:

.form-group-sm .form-control 
{
    &:extend(.input-sm);
}
.form-group-sm select.form-control 
{
    &:extend(select.input-sm);
}

.form-group-sm select[multiple].form-control 
{
    &:extend(select[multiple].input-sm);
}

.form-group-sm textarea.form-control 
{
    &:extend(textarea.input-sm);
}

Do the same for lg styles. Not very elegant and a little verbose, but because of how the mixins written I don't think there are a lot of options.

glebm added a commit to twbs/bootstrap-sass that referenced this issue Dec 17, 2014

@cvrebert cvrebert added the confirmed label Jan 12, 2015

glebm added a commit to twbs/bootstrap-sass that referenced this issue Jan 18, 2015

@mdo mdo modified the milestones: v3.3.2, v3.3.3 Jan 19, 2015

@cvrebert cvrebert modified the milestones: v3.3.5, v3.3.4 Mar 15, 2015

@mdo mdo closed this in df8010b Mar 29, 2015

mdo added a commit that referenced this issue Mar 29, 2015

rroblak pushed a commit to insight-meditation-center/imc-bootstrap-sass that referenced this issue Nov 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.