-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(list-box): remove 32px margin right on option #5400
fix(list-box): remove 32px margin right on option #5400
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @abbeyhrt for your quick fix!
Deploy preview for carbon-elements ready! Built with commit 10bb00a |
Deploy preview for carbon-components-react ready! Built with commit 10bb00a https://deploy-preview-5400--carbon-components-react.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 5d9802d |
Deploy preview for carbon-components-react ready! Built with commit 5d9802d https://deploy-preview-5400--carbon-components-react.netlify.com |
@@ -535,7 +535,7 @@ $list-box-menu-width: rem(300px); | |||
|
|||
.#{$prefix}--list-box.#{$prefix}--list-box--inline | |||
.#{$prefix}--list-box__menu-item__option { | |||
margin: 0 rem(32px) 0 $carbon--spacing-03; | |||
margin: 0 $carbon--spacing-03; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like removing the padding affects the border-bottom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to reproduce that but it shouldn't. just to clarify I am reducing (not removing) the padding. but if we were to remove the padding the width (and border) would still be unaffected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh could have sworn that image had a border going all the way to the right 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah if it was just the image then it was a bad screenshot. you can test it locally by changing the padding in devtools. the margin is what will determine where the border is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had just skimmed the PR and saw that. Now that I've checked it out I agree, we can reduce this to 1.5rem
and have the entire Option
text show, but still leave space for the checkmark to ensure text does not overflow under the icon.
padding-right: $carbon--spacing-06;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the change! I was hoping to get design insight to see if when they wanted the text to truncate but I think I'm probably good to just make the change
Closes #5389
This removes the 32px margin-right on the inline listbox's that was recently added in #4753.
@asudoh I'm sure that was added for a reason, I just couldn't find any negative visual change when removing it so let me know if I'm missing something!
It also seems like the text is truncating a bit too soon but wanted the opinion of design: