fix(form): update label alignment

[christiemolloy]

closes #2167

Spoke with @kybaker about how we should handle form labels. We came to the conclusion that: -

• for form labels adjacent to a form control with one line, the label should be vertically aligned in the center.
• for form labels adjacent to a form control with two lines, the label should be aligned to the top.

This should allow for these labels to more flexible, especially with font-sizes changing. The downside is that the user will have to add a modifier to change the label to align to the top.

Here is the example in the horizontal form demo:

Bootstrap does something similar:

[patternfly-build]

PatternFly-Next preview: https://patternfly-next-pr-2200.surge.sh

[mcoker]

form--id needs to be unique

[mcoker]

If a user has a horizontal group with multiple form control elements, or just a tall element, the label is going to be vertically aligned to the center. Is that OK or would that be breaking?

This is what that will look like if a user just pulls in this PR without adding the pf-m-align-top class.

And personally I think labels adjacent to groups with multiple lines where the first line is a form control element, I would still expect the label text to align with the text in the form control element.

So this (top padding)

instead of this (no padding)

[christiemolloy]

@mcoker Yeah I think that will be a breaking change, would you recommend that I make this an opt-in solution or release it during the breaking change period?

In the screenshot below is this how you would expect to see it happen? By default the label is vertically-aligned in the center (helper text is outside of pf-c-form__horizontal-group) so that it doesn't make the form control taller than it needs to be. For labels adjacent to a text area they can use the modifier pf-m-align-top so that is aligned to the top and has a top padding so that the text is aligned horizontally?

[mcoker]

In the screenshot below is this how you would expect to see it happen?

It is, but that's just my opinion :)

If we went that way, couldn't we just add a pf-m-no-padding-top modifier to the existing form labels that removes the top padding when the label is adjacent to something that isn't a form control element?

I was also going to say that we could also just apply the padding-top that is applied to labels to both the label and the horizontal form group to the right, then say .pf-c-form__horizontal-group > .pf-c-form-control:first-child, .pf-c-form__horizontal-group > .pf-c-select:first-child, [etc] { margin-top: -[the padding top of the horizontal group]; } to offset the padding when one of those components that uses --pf-global--spacer--form-element as the top padding is the first child of the horizontal form group. I don't actually think we should do that, as I think it could get tricky and we might end up with a really long list of components in the selector, but that would be one way to make the same adjustment and a user wouldn't need to use a modifier for the label and content in the horizontal form group to align.

[christiemolloy]

@mcoker my thoughts on the padding for alignment is that it isn't the most flexible option especially when font-sizes change

@kybaker what are your thoughts on having the label that is aligned to the top, have a padding-top? Or should it just be aligned to the top?

[christiemolloy]

@mcoker @mattnolting made updates so that this isn't breaking... adding a modifier for when the label is adjacent to a non-formcontrol element

Also confirmed with @mcarrano that wrapping the label is fine, but from a design perspective long labels should be avoided.

[mcoker]

Suggested change

	padding-top: 0;
	--pf-c-form__label--PaddingTop: 0;

[christiemolloy]

updated

[mcoker]

should this be .pf-m-no-padding-top?

[christiemolloy]

updated

[mcoker]

There is some extra space under the textarea for some reason...

Also since we have a helper text example, I wonder if we should remove that and just show helper text in the vertical and horizontal examples.

[mcoker]

This looks great to me!! I left a couple of comments. We can address in this PR if you want, or we can open a new issue to look at later.

[christiemolloy]

Thanks for reviewing. I think I captured all of your comments and removed the text underneath

[mattnolting]

Great work! LGTM

[redallen]

🎉 This PR is included in version 2.31.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀