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.

Sign up for GitHub

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

Jump to bottom

fix(list-box): remove 32px margin right on option #5400

Merged
merged 6 commits into from
Feb 20, 2020
Merged

fix(list-box): remove 32px margin right on option #5400

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10bb00a
fix(list-box): remove 32px margin right on option
abbeyhrt Feb 19, 2020
69afa85
Merge branch 'master' into inline-listbox-5389
abbeyhrt Feb 19, 2020
63cc5d2
Merge branch 'master' into inline-listbox-5389
tw15egan Feb 20, 2020
8eb2fcf
fix(list-box): reduce padding right by 8px
abbeyhrt Feb 20, 2020
0f32181
fix(list-box): reduce padding right by 8px
abbeyhrt Feb 20, 2020
5d9802d
Merge branch 'master' into inline-listbox-5389
abbeyhrt Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/src/components/list-box/_list-box.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

@emyarod emyarod Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the items are still cut off early, should we reduce the right padding by 8px as well? I think this will account for the margin and resolve the issue

image

with reduced right padding:

image

Copy link
Collaborator

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

Copy link
Member

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

Copy link
Collaborator

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 🙃

Copy link
Member

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

Copy link
Collaborator

@tw15egan tw15egan Feb 20, 2020 •
edited
Loading

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;

Copy link
Contributor Author

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


&:focus {
margin: 0;
Expand Down