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(toggle): change spacing to match style guide #5462

Merged
merged 7 commits into from
Mar 6, 2020

Conversation

FrivalszkyP
Copy link
Contributor

Changelog

Changed

Testing / Reviewing

Please validate that the wording in the style guide matches the changes that I implemented.

Please note that I checked with my UI designer colleague and we found that the toggle button in the design kit does not have a bottom margin so I decided to omit it here as well. It seems more consistent to me.

@FrivalszkyP FrivalszkyP requested a review from a team as a code owner February 27, 2020 12:19
@ghost ghost requested review from asudoh and dakahn February 27, 2020 12:19
@tw15egan
Copy link
Collaborator

Related: #5367

It seems like it is getting the margin, even though no label is present

Screen Shot 2020-02-27 at 1 31 47 PM

I think I'm leaning towards removing the spacing completely until we refactor form element spacing in v11.

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-components-react ready!

Built with commit fa87fbc

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

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-elements ready!

Built with commit fa87fbc

https://deploy-preview-5462--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-components-react ready!

Built with commit 67461e3

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

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-elements ready!

Built with commit 67461e3

https://deploy-preview-5462--carbon-elements.netlify.com

@FrivalszkyP
Copy link
Contributor Author

It seems like it is getting the margin, even though no label is present

Right. It seems like there's no way to write a selector for toggle switches that don't have a label.

I think I'm leaning towards removing the spacing completely until we refactor form element spacing in v11.

Not a bad idea, should I refactor my PR or is there an ongoing effort to clean up the random spacing issues all throughout the codebase?

@tw15egan
Copy link
Collaborator

No current efforts but it's been added to our planning. We can see what the other devs think in regards to removing this margin now though since you've already got the PR up.

@asudoh asudoh requested review from a team and designertyler and removed request for a team February 28, 2020 22:38
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @FrivalszkyP!

@asudoh asudoh merged commit 606ea32 into carbon-design-system:master Mar 6, 2020
joshblack pushed a commit to joshblack/carbon that referenced this pull request Mar 10, 2020
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.

5 participants