Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Fix: Same margin than text-field in boxed style #2945

Closed
wants to merge 7 commits into from

Conversation

vokimon
Copy link

@vokimon vokimon commented Jun 18, 2018

Using boxed style, Text-fields have a 16px margin than selects haven't. If you layout them horizontally (in the same row), they shown distinct vertical positions. If you layout them vertically, the spacing between fields is irregular. This adds the same margin-top to the selects than it is defined for the textfields.

Using boxed style, Text-fields have a 16px margin than selects haven't. If you layout them horizontally (in the same row), they shown distinct vertical positions. If you layout them vertically, the spacing between fields is irregular. This adds the same margin-top to the selects.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@patrickrodee
Copy link
Contributor

Thanks for the PR, @vokimon! We always appreciate community contributions.

After discussing this, we'd like to actually do the inverse of your PR. Let's remove the margin-top (and margin-bottom) from the MDC Text Field component. I'll explain why below.

Having margin on the top-level element of a component may make it look nice in general, but quickly becomes problematic when used in any custom context. The margin can easily cause layout problems where a component can't be vertically lined up next to a horizontally adjacent item. That's no good as it makes assumptions about the context in which the component is used.

By removing the margin from the top-level elements, we make no assumptions about the surrounding context of the component. Instead, the user can apply margin as they need it.

If you could update your PR with those changes (remove margin-top and margin-bottom from MDC Text Field and revert the margin-top addition to MDC Select), that would be great!

Also, make sure you sign the CLA 👍

@patrickrodee patrickrodee self-assigned this Jun 20, 2018
@vokimon
Copy link
Author

vokimon commented Jun 22, 2018

Indeed i agree a lot with your proposal. On it.

@googlebot
Copy link

CLAs look good, thanks!

@vokimon
Copy link
Author

vokimon commented Jun 22, 2018

I just removed the margin-top. Not sure if there is any margin-bottom. Besides the examples i am running just exercise the boxed style.

@williamernest
Copy link
Contributor

Thanks for filing this PR. We ended up getting rid of the text field margin as part of other updates to match spec in #3397.

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

Successfully merging this pull request may close these issues.

5 participants