view_components icon indicating copy to clipboard operation
view_components copied to clipboard
verified publisher icon primer

Form elements do not have sufficient vertical spacing

Open HDinger opened this issue 1 year ago • 3 comments

Steps to reproduce

  • Create a form with many elements
  • Observe the spacing between the elements

Actual behavior

  • The individual fields are very close together, there is only little vertical space between them. That does not look very nice and is also hard for users to scan.
    • The space is not consistent, it is larger, when the field above has a description displayed.
    • The FormControl-spacingWrapper adds a spacing of 8px in between the elements which is not enough imho
Bildschirmfoto 2024-08-30 um 13 50 27
  • It feels like in GitHub the same issue was experienced and "solved" by putting a FormGroup around each individual field to add more spacing (see for example https://github.com/settings/profile)
Bildschirmfoto 2024-08-30 um 13 52 32

Expected behavior

  • In my eyes, it should not be required to put FormGroups around individual elements to make a comparably simply form readable
  • The default spacing between form elements should be larger (e.g 16px).

HDinger avatar Aug 30 '24 11:08 HDinger

Thanks for bringing this up @HDinger 😄 If you have time to put together a PR for this, I would be happy to review it. You're right that we shouldn't have to wrap each form field in a group wrapper, although the framework does that automatically. It might be worth removing caption padding and increasing the padding on the group element. Thoughts?

camertron avatar Sep 04 '24 18:09 camertron

Hi @camertron I gave it a shot in https://github.com/primer/view_components/pull/3087. I'd be happy to get your review on this. I for now tried to simply increase the space which is given by the FormControl-spacingWrapper because of two reasons:

  1. The spacingWrapper is meant exactly for this case. It used a custom space and I replaced it with a standardized variable.
  2. Groups are not always added automatically. Basically every expample that is in the lookbook does not have a form group, so we cannot rely on the group to take care of the spacing

HDinger avatar Sep 13 '24 07:09 HDinger

I don't think form elements should project padding/margins. The spacing between form controls should be handled by the layout CSS in whatever context you're rendering the form controls in.

mperrotti avatar Sep 23 '24 16:09 mperrotti

This can be closed as it is tackled in https://github.com/primer/view_components/pull/3159

HDinger avatar Oct 28 '24 07:10 HDinger