Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Using a custom icon breaks the layout of the Carousel component #92

Closed
francoismassart opened this issue Nov 7, 2017 · 6 comments
Closed

Comments

@francoismassart
Copy link
Contributor

Due to a CSS selector not getting applied:

.carousel-control .icon-prev,
.carousel-control .icon-next,
.carousel-control .glyphicon-chevron-left,
.carousel-control .glyphicon-chevron-right

PR is coming

@wxsms
Copy link
Member

wxsms commented Nov 8, 2017

Merged.

wxsms added a commit that referenced this issue Nov 8, 2017
@wxsms
Copy link
Member

wxsms commented Nov 8, 2017

Hi @francoismassart

Thanks for reporting this issue and the PR, actually I have never try this prop before.

But you solution is causing all icons become icon-prev and icon-next, including the default one. I just do some research and refine it. Seems custom CSS may needed in some case. You can check out my commit for details.

@francoismassart
Copy link
Contributor Author

Yes, indeed. Because we cannot edit the official/external CSS file from bootstrap for this issue, here is what I propose as solution: we only concatenate the icon-prevand icon-next classes to the HTML node of the prev/next if their defined value is different from the default...

This way, if we want to use different/custom icons it is our responsability to edit our custom CSS declarations with just enough specificity to overwrite the specific inherited & unwanted CSS properties coming from the official bootstrap CSS.

Sounds good ?

@wxsms
Copy link
Member

wxsms commented Nov 8, 2017

image

icon-prev / icon-next is overriding the font-family, and also the before font content. And, it also might not look good while the new icon has more height.

While not using them, all we need to add in CSS are:

.carousel-control .your-icon {
  top: 50%;
  margin-top: -10px; // half of the new icon height
  position: absolute;
}

I think this might be easier. Might also good to have in document.

@francoismassart
Copy link
Contributor Author

francoismassart commented Nov 8, 2017

I'll make the needed changes in a PR

@wxsms
Copy link
Member

wxsms commented Nov 8, 2017

Thanks. Just enhance the doc will be enough for now. pls pull latest code first.

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

No branches or pull requests

2 participants