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 Issue #15691] .form-control-static changes height when empty #15699

Merged
merged 1 commit into from
Feb 19, 2015

Conversation

kkirsche
Copy link
Contributor

[Fix Issue #15691] .form-control-static changes height when empty by setting the minimum height to the size it is when text is in the .form-control-static

JSFiddle: http://jsfiddle.net/4pdo4yzo/

@cvrebert cvrebert added the css label Jan 29, 2015
@mdo
Copy link
Member

mdo commented Jan 29, 2015

The value of 34px needs to be calculated based on the font-size, line-height, etc. I'm out of town right now, but if you don't get to it, I probably can next week.

@kkirsche
Copy link
Contributor Author

Calculating it now with line height and font-size-base. Thanks for your feedback

@cvrebert
Copy link
Collaborator

Do we need to do anything extra to cover the .form-group-* cases too? (e.g.

.form-control-static {
)

@kkirsche
Copy link
Contributor Author

I don't believe so @cvrebert but I'd love to hear from others if they think so.

My logic for why it wouldn't be necessary is that as the .form-group-* cases are not overriding or declaring a min-width value, the declaration for all .form-control-static, which the min-height was set on in this, should apply to those further down the line.

@kkirsche
Copy link
Contributor Author

Modified the jsfiddle to show what I mean:

http://jsfiddle.net/m7vnxh7u/

EDIT: Nevermind, I see why we would want it on those. I'll make those changes 👍

@twbs-lmvtfy
Copy link

Hi @kkirsche!

You appear to have posted a live example (http://fiddle.jshell.net/m7vnxh7u/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 51, column 3: E014: Columns (.col-*-*) can only be children of .rows or .form-groups
  • line 52, column 3: E014: Columns (.col-*-*) can only be children of .rows or .form-groups

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.)

@kkirsche
Copy link
Contributor Author

Fixed. Good catch, I messed up the logic behind this as it should be different for each version of the form-group to adjust for the different font-sizes of each .form-group-* type

@kkirsche
Copy link
Contributor Author

kkirsche commented Feb 3, 2015

Squashed commits into single due to small size of this change and it seems to have stabilized in regards to feedback.

@kkirsche
Copy link
Contributor Author

Just wanted to follow up on this and see if anything was missing.

@kkirsche
Copy link
Contributor Author

Wanted to follow up about this and just check if anything else was necessary. Thanks for your time guys 👍

@mdo
Copy link
Member

mdo commented Feb 19, 2015

:shipit:

@cvrebert
Copy link
Collaborator

Will merge this ASAP; I just need to rebase it to adjust the commit message.

@kkirsche
Copy link
Contributor Author

Rebased and fixed @cvrebert

[Fixes twbs#15691] .form-control-static changes height when empty by setting the minimum height to the size it is when text is in the `.form-control-static`

JSFiddle: http://jsfiddle.net/4pdo4yzo/

Compute the minimum height

@line-height-computed = ~20px
@font-size-base = 14px

This should come to approx. 34px

Add min-height to from-group-* per @cvrebert

I had this wrong. The minimum height would depend on the font-size of that specific group, thus we need to declare it once for font-size-small and once also for font-size-large
@cvrebert
Copy link
Collaborator

@kkirsche Thanks man!

cvrebert added a commit that referenced this pull request Feb 19, 2015
Fix #15691: .form-control-static changes height when empty
@cvrebert cvrebert merged commit 2549b80 into twbs:master Feb 19, 2015
@cvrebert cvrebert mentioned this pull request Feb 19, 2015
@kkirsche kkirsche deleted the patch-8 branch February 19, 2015 23:57
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.

5 participants