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

RFC: Adjust padding and margins on Input #324

Open
guyfedwards opened this issue Jul 26, 2018 · 9 comments
Open

RFC: Adjust padding and margins on Input #324

guyfedwards opened this issue Jul 26, 2018 · 9 comments
Labels
component/form-elements Issues related to atomic form element components needs-discussion Issue is a question or a proposal that needs discussion style-guide Part of the effort towards a unified design system

Comments

@guyfedwards
Copy link
Contributor

guyfedwards commented Jul 26, 2018

Currently Input has padding and margins on the input itself which are really implementation details, and shouldn't be handled by the Input itself.
As a result I find myself manually removing/changing the margins/paddings in a lot of circumstances.

A margin-bottom for spacing makes sense, but having a top as well and relying on collapsing margins when the label is not there doesn't seem like the cleanest solution.

We want to have an Input component that can be used universally without impacting its surroundings and with minimal overrides.

I am unsure of why said padding/margins are all there so want to open the discussion around this.

image
image

Ideally we could also move all positional and box-model related css properties to the root component to allow for easier overrides when we do inevitably have to.
E.g. the below is a nested component within Input but the margin has impact on the implementations position, this would be remedied by moving the spacing to the root component and allowing overrides to be applied there.

image

@guyfedwards guyfedwards added the needs-discussion Issue is a question or a proposal that needs discussion label Jul 26, 2018
@bowenli
Copy link
Contributor

bowenli commented Jul 26, 2018

Removing margin makes sense because components shouldn't influence layout.
Having padding ensures consistency. @guyfedwards Have you needed to override the padding?

@guyfedwards
Copy link
Contributor Author

Largely vice-versa, I have had to remove the padding and leave the margin.

For example, the input has a margin-bottom: 8px which I imagine is there to space between the input itself and a validation message that might show up.
I have an implementation where I am trying to add a message below the input component, which should be the consistent 8px below, but I have both the margin and the padding, giving a total spacing of 18px and thus I must override to make it consistent with the 8px used in other components.
image

I suppose the issue may be more around when and how we apply the padding, as opposed to having it at all which, as you mention and I agree, ensures consistency. The margin below the input for example, should really be on the validation message so that it is only present when the validation message is showing.

The right-padding on an input is there to consistently space between inputs I would assume? So it might make more sense to move it from being always there, to being left-padding only on inputs that are next to other inputs. This way when you want to have a full width input you don't need to remove the right padding.

I have been using the inputs for a short space of time and only in two situations but in both I have found myself needing to tweak and remove padding/margins so wanted to open the discussion to find out more around what the spacing/layout rules are and how the input/form components should interact with each other, then we can look to tweak the implementation a little to result in less custom code being written for these components.

@guyfedwards
Copy link
Contributor Author

A follow up question:
Is there somewhere I can see design docs for this kind of stuff? Padding/layouts/componetns etc?

@guyfedwards guyfedwards changed the title RFC: Remove padding and margins from Input RFC: Adjust padding and margins on Input Jul 27, 2018
guyfedwards added a commit that referenced this issue Jul 27, 2018
Only show the message if there is a message value *and* the field
is invalid.
Move the margin for spacing between input and message to the message
itself so the space is not there when the message is not.

related #324
@bowenli
Copy link
Contributor

bowenli commented Jul 30, 2018

The 12px padding-right and padding-left to position the text entered inside the <input> - it shouldn't affect how the component is positioned wrt other elements. Is that what you are talking about?

@bowenli
Copy link
Contributor

bowenli commented Jul 30, 2018

Re is there a doc, this repo is it for components. On layout, etc. we don't have anything completed as of now.

@guyfedwards
Copy link
Contributor Author

Regarding the padding, yep exactly. Currently if you want a full width input, you need to remove the right padding, for example. What I have had success with in the past is to create a wrapper element ('input-group' for example), that handles the layout of input elements when there is more than one.

You want to use an input straight up, no problem, it doesn't have any opinions about how it is spaced.
You want to have a series of horizontal or vertically organised inputs, it would add the paddings between the elements on the right and bottom respectively.

@bowenli
Copy link
Contributor

bowenli commented Jul 31, 2018

Full width input - even if the text goes beyond the input width, it's still appropriate to have padding on the right.

Additional inputs on the right - the padding shouldn't affect this. Can you give an example of where this is happening?

@guyfedwards
Copy link
Contributor Author

Apologies, we're talking at cross purposes here as I wasn't very clear, there is padding in the input element and thats fine, but there is padding surrounding the whole larger element (input, label and message) as well, screenshot for clarity:

image

@guyfedwards
Copy link
Contributor Author

Part of this tweaking and adjustment should include changing this 'padding' to margin as it is for spacing between elements

@fbarl fbarl added style-guide Part of the effort towards a unified design system component/form-elements Issues related to atomic form element components labels Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/form-elements Issues related to atomic form element components needs-discussion Issue is a question or a proposal that needs discussion style-guide Part of the effort towards a unified design system
Projects
None yet
Development

No branches or pull requests

3 participants