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

PR #25944 broke custom-control-label when empty #26453

Closed
qraynaud opened this issue May 4, 2018 · 9 comments
Closed

PR #25944 broke custom-control-label when empty #26453

qraynaud opened this issue May 4, 2018 · 9 comments
Labels

Comments

@qraynaud
Copy link

qraynaud commented May 4, 2018

Empty labels are needed when using checkboxes in horizontal forms. If you let it empty in this use case on 4.1.1, the checkbox appears way off in the bottom: https://codepen.io/anon/pen/ZoJqyd

As you can see, it was working perfectly on 4.1.0: https://codepen.io/anon/pen/VxzEMp

I tried to look into every commit between 4.1.0 and 4.1.1 and found that PR #25944 was to blame. When I reverted its changes, the behavior was as before. Because this fix looked more than useful, I found a way to keep it and get my use case working as intended.

My proposed fix is to add vertical-align: top to the custom-control-label class:

.custom-control-label {
  vertical-align: top;
}

Here is a demo: https://codepen.io/anon/pen/NMvLEw

Since I don't know the bootstrap codebase & good practices, I hope someone else can come up with a PR though.

@asyncdesign
Copy link

asyncdesign commented May 6, 2018

The class custom-control-label has suddenly adopted position: relative - this was not in 4.1.0.

Also, you might want to check what reboot.scss is doing exactly.

label {
    display: inline-block;
    margin-bottom: .5rem;
}

I would use display: block for this purpose since it is a custom control, then you can use position: relative as well, and margin-bottom: 0.5rem would only be appropriate for one specific control size;

@qraynaud
Copy link
Author

qraynaud commented May 7, 2018

@asyncdesign : as I stated, the bug was introduced on 4.1.1, not 4.1.0. Please read me carefully. And I provided codepens using the default bootstrap builds so my reboot.scss cannot be causing this in any way!

Here is the problematic commit : fc0fcc4.

@asyncdesign
Copy link

asyncdesign commented May 7, 2018

I know reboot isn't causing this issue, but just wanted to point out potential issues with the label having margin-bottom: .5rem, and having display: inline-block with position: relative doesn't make sense for a label. This was mainly for the Bootstrap devs to think about. But, if vertical-align: top was added to Bootstrap as you suggest, what will happen if the font-size is increased to say 1.3em? I would think that the text should be centered vertically within the label whatever the line-height is - same goes for the custom control.

@qraynaud
Copy link
Author

qraynaud commented May 7, 2018

@asyncdesign : ok, sorry to have misunderstood you on this point.

But I don't see the problem here though. The .custom-control-label class is resetting margin-bottom to 0 anyway (see here: https://github.com/twbs/bootstrap/blob/v4-dev/scss/_custom-forms.scss#L61).

So I don't think it's related. But I agree about the intended alignment that would be broken with an increased font-size though.

I have to note on this subject that it is already broken without vertical-align: top right now as you can see here: https://codepen.io/anon/pen/MGOpLrk. Adding vertical-align: top does nothing in this use case so my fix seems okay in the current state of things.

Maybe we should fill another bug about the vertical alignment of checkbox & text in this use case? But I'm under the impression that custom controls do not support changing font size at all (seeing how they're implemented right now). I think that ideally, the checkbox should appear 1.3× bigger if 1.3em is used (rather than aligning properly the items).

@ysds
Copy link
Member

ysds commented May 7, 2018

The cause of this bug is a lack of consideration for vertical alignment when no text in elements. I created a simple text case in https://codepen.io/anon/pen/yjPbxG.

So an easy solution is adding vertical-align: top to .custom-control-label.

Demo: https://codepen.io/anon/pen/GdOZjN

But probably the ideal solution it is to rewrite to flexbox (#25184 as well as #25337 and #25828).

I'd like to hear advice from @mdo before creating a PR.

@mdo
Copy link
Member

mdo commented May 10, 2018

If rewriting it in flexbox solves this issue @ysds, I'd prefer that. It'd give folks even more control through utility classes.

@qraynaud
Copy link
Author

qraynaud commented May 10, 2018

I agree that flexbox would probably solve most of the issues we are seeing with those and that it seems a much better solution overall.

It might be a good idea to land a vertical-align: top fix in the meantime to fix the regression between 4.1.0 & 4.1.1 though. That's an easy temporary fix.

What are your thoughts on this @mdo?

@ysds
Copy link
Member

ysds commented May 11, 2018

@mdo Thank! But the flex approach probably should be included in a minor (or major) update, because If the flexbox approach is successful (this means remove position: absolute), the following position-static class is not needed.

Without labels (Native):

Add .position-static to inputs within .form-check that don’t have any label text.

<div class="form-check">
  <input class="form-check-input position-static" type="checkbox">
</div>

So, as @qraynaud said, it is better to add vertical-align: top for quick fix in 4.1.2.

@samuel02
Copy link

@ysds #25944 also breaks checkboxes in tables, see: https://codepen.io/anon/pen/mzLdwj .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants