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(radio-button): margin bug fix #7133

Merged
merged 4 commits into from Nov 12, 2020
Merged

fix(radio-button): margin bug fix #7133

merged 4 commits into from Nov 12, 2020

Conversation

andreancardona
Copy link
Contributor

@andreancardona andreancardona commented Oct 26, 2020

Closes #7113

added a margin around radio button to fix truncation bug

Changelog

New

  • 0.25 rem to margin-top & margin-bottom around radio-button

Testing / Reviewing

Screen Shot 2020-10-26 at 11 46 41 AM

@netlify
Copy link

netlify bot commented Oct 26, 2020

Deploy preview for carbon-elements ready!

Built with commit 6a018f1

https://deploy-preview-7133--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 26, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 6a018f1

https://deploy-preview-7133--carbon-components-react.netlify.app

@joshblack
Copy link
Contributor

@andreancardona if I understand this issue, it's that the box-shadow bleeds over the box model of the radio button? Just wanted to confirm 👀

@andreancardona
Copy link
Contributor Author

andreancardona commented Oct 29, 2020

@andreancardona if I understand this issue, it's that the box-shadow bleeds over the box model of the radio button? Just wanted to confirm 👀

Yep! Let me know if there is a better way to solve for it

@joshblack
Copy link
Contributor

@andreancardona this makes me think about the focus style itself, I'm curious if the shadow should be inset then instead? Would require a sync with design but it seems like our focus styles should probably stay scoped inside of the container instead of growing otherwise we do have to add in some extra spacing to accommodate for it 🤔

@andreancardona
Copy link
Contributor Author

kes me think about the focus style itself, I'm curious if the shadow should be inset then instead? Would require a sync with design but it seems like our focus styles should probably stay scoped inside of the contai

Yeah that makes a lot more sense. What is the best next step then in this situation?

…scss

Co-authored-by: Josh Black <josh@josh.black>
@joshblack
Copy link
Contributor

bump @dakahn if you have a sec to review today

@tw15egan
Copy link
Member

tw15egan commented Nov 5, 2020

Just curious, if we're adding a margin to the top and bottom to account for the focus, should we also add some to the left side?

Screen Shot 2020-11-05 at 11 47 52 AM

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

seems cool to me, pending @tw15egan 's questions

@andreancardona
Copy link
Contributor Author

Just curious, if we're adding a margin to the top and bottom to account for the focus, should we also add some to the left side?

Screen Shot 2020-11-05 at 11 47 52 AM

good catch! updated!

@kodiakhq kodiakhq bot merged commit 5e9b3fc into carbon-design-system:master Nov 12, 2020
@andreancardona andreancardona mentioned this pull request Nov 13, 2020
59 tasks
emyarod added a commit to emyarod/carbon that referenced this pull request Feb 24, 2021
kodiakhq bot added a commit that referenced this pull request Feb 24, 2021
…#7886)

* Revert "fix(radio-button): margin bug fix (#7133)"

This reverts commit 5e9b3fc.

* fix(radio-button): avoid outline cutoff in hidden overflow containers

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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.

Radio button's focus state is truncated when in a container with a hidden overflow
4 participants