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

fix: form spacing #4576

Closed
wants to merge 2 commits into from

Conversation

breid1313
Copy link
Contributor

Closes #2343

Changelog

Changed

  • added 24px/1.5 rem margin-bottom to form--form-item using the $carbon--spacing-06 following the Form Style Guide
  • adjusted the margin-bottom on form--fieldset to $carbon--spacing-06 for consistency

Testing / Reviewing

Storybook before (small snippet):
Screen Shot 2019-11-05 at 3 57 01 PM

Storybook after (small snippet):
Screen Shot 2019-11-05 at 3 56 10 PM

@breid1313 breid1313 requested a review from a team as a code owner November 5, 2019 20:57
@ghost ghost requested review from emyarod and joshblack November 5, 2019 20:57
@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for the-carbon-components ready!

Built with commit c97cce7

https://deploy-preview-4576--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-elements failed.

Built with commit c97cce7

https://app.netlify.com/sites/carbon-elements/deploys/5dc2243359201300083a33bf

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-components-react ready!

Built with commit c97cce7

https://deploy-preview-4576--carbon-components-react.netlify.com

@asudoh asudoh requested a review from a team November 5, 2019 22:37
@ghost ghost requested review from aagonzales and removed request for a team November 5, 2019 22:37
@@ -29,6 +29,7 @@
// `flex-direction: row`
flex: 1 1 auto;
align-items: flex-start;
margin-bottom: $carbon--spacing-06;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding this to bx--form-item unexpected add margin for folks using this control? Might be good to offer it by default, but might catch folks off guard if we add this layout potentially

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think this is part of a discussion that's been on hold since the v10 effort began

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dev thinks its ok to update without breaking people then it good to me.

@joshblack
Copy link
Contributor

Coming back to this, looks like we can't update it without causing a breaking change for certain users 😞Went ahead and added a v11 label to the corresponding issue so we make sure to address this in our next update.

Just wanted to say thanks so much for your time and effort here, really appreciate it 🙏 Sorry that things took so long and ended up the way that they did 😞

@joshblack joshblack closed this Dec 5, 2019
@breid1313
Copy link
Contributor Author

@joshblack no need to apologize! Happy to (continue to) help where I can 👍 . Thanks for following up 😄

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

Successfully merging this pull request may close these issues.

Form spacing inconsistencies
6 participants