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 carousel-control margin asymmetry #13242

Merged
merged 1 commit into from
Apr 7, 2014

Conversation

killthekitten
Copy link
Contributor

While using carousel default controls with a non-default background, I've noticed annoying asymmetry of arrows. Looks like somebody forgot to specify different margins for left and right arrows.

@killthekitten killthekitten changed the title Fix carousel control margin asymmetry Fix carousel-control margin asymmetry Mar 31, 2014
@cvrebert cvrebert added the css label Mar 31, 2014
@cvrebert cvrebert added this to the v3.2.0 milestone Mar 31, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

Please provide a demonstration of the problem and your fix. Would be much appreciated 😁

@killthekitten
Copy link
Contributor Author

@hnrch02 at first I attached screenshots, but then my team kicked me for using production data 👍 So here they are, v. 2.

Before:
2014-03-31 12 24 47

After:
2014-03-31 12 35 32

@killthekitten
Copy link
Contributor Author

@hnrch02 probably, some extra-testing is needed, to be honest I don't know how to test it well

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

Ah, yeah, I understand now. To further clarify what this does:
Carousel controls margin

@killthekitten
Copy link
Contributor Author

@hnrch02 exactly. Also, I don't understand why there's still 2-pixels (51 vs 49) width difference between both areas?

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

That's because the chevrons aren't actually as wide as their containers and I measured the distance directly from them. If we'd take the width of the containers into account their is only a difference of one pixel, as you can see here:
chevron-containers

The one pixel actually seems to originate from a rounding difference in the width: 15% of the .carousel-control containers:
rounding

@killthekitten
Copy link
Contributor Author

@hnrch02 now it's clear to me, thx. And what about PR?

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

I made the viewport smaller and checked again, the .carousel-controls both being 100px wide, and the one pixel still persists – doesn't seem like a rounding problem. If we'd set the margin-right of the right chevron to -16px the problem vanishes on all viewport sizes:
example

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

There's nothing I can do about the PR, I'm just some guy participating in the discussion. Need @mdo on this.

@killthekitten
Copy link
Contributor Author

@hnrch02 do we also need to change margin-right from 10px to 11px for the xs-sm screens (15px margin is wrapped with @media screen and (min-width: @screen-sm-min))?

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 31, 2014

Oddly enough, the small viewports are not affected:
small-viewport
So no change needed for these sizes.

mdo added a commit that referenced this pull request Apr 7, 2014
@mdo mdo merged commit 1554b7c into twbs:master Apr 7, 2014
@mdo mdo mentioned this pull request Apr 7, 2014
@killthekitten
Copy link
Contributor Author

@mdo 👍 thank you. But I forgot to add #13293

@elkangaroo
Copy link

Thx for the fix @killthekitten, just stumbled upon this in my carousel as well.

@killthekitten
Copy link
Contributor Author

@elkangaroo 👍 you're welcome

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

Successfully merging this pull request may close these issues.

None yet

5 participants