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

Simpler carousel indicators css #26996

Merged
merged 8 commits into from
Aug 26, 2018

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Jul 30, 2018

Pseudo elements are used to increase the hit area by 10px on top and bottom, but this can also be done with transparent borders which saves a bit css.

Demo: https://codepen.io/MartijnCuppens/pen/ajNXZj

@XhmikosR
Copy link
Member

XhmikosR commented Aug 2, 2018

I definitely like this, but better wait for one more approval.

@XhmikosR
Copy link
Member

Ping @andresgalante @mdo

margin-right: $carousel-indicator-spacer;
margin-left: $carousel-indicator-spacer;
text-indent: -999px;
cursor: pointer;
background-color: $carousel-indicator-active-bg;
background-clip: padding-box;
// Use transparent borders to increase the hit area by 10px on top and bottom.
border-top: 10px solid transparent;
Copy link
Member

Choose a reason for hiding this comment

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

Can we move to a variable for the 10px value, and then use that here, on L196 and L187?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to a variable and used box-sizing: content-box instead of adding the hit area height to indicator height (this makes it possible to define the hit area height in another unit).

@XhmikosR XhmikosR merged commit 0e88315 into twbs:v4-dev Aug 26, 2018
@mdo mdo mentioned this pull request Aug 26, 2018
@MartijnCuppens MartijnCuppens deleted the carousel-indicator-cleanup branch August 26, 2018 12:07
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.

4 participants