[WIP] v5: Carousel improvements for easier customization #28266
Conversation
XhmikosR
changed the title
XhmikosR
force-pushed
the
v5-carousel
branch
from
19de417
to
072a0e0
Compare
XhmikosR
force-pushed
the
v5-carousel
branch
2 times, most recently
from
303c42c
to
05a439c
Compare
…nge text, control, and indicator colors
|
Cleaned some of this up for the new example I added many months ago. Any additional ideas for what to improve here for v5 @twbs/css-review? |
| display: block; | ||
| height: $carousel-indicator-hit-area-height * 3; | ||
| content: ""; | ||
| background-color: rgba(255, 0, 0, .1); |
There was a problem hiding this comment.
I think this shadow needs to be applied to the element itself
| } | ||
| .carousel-control-prev-icon { | ||
| background-image: escape-svg($carousel-control-prev-icon-bg); | ||
| // background-image: escape-svg($carousel-control-prev-icon-bg); |
There was a problem hiding this comment.
The controls are gone now?
| @@ -158,21 +161,35 @@ | |||
| list-style: none; | |||
|
|
|||
| li { | |||
| box-sizing: content-box; | |||
| position: relative; | |||
| // box-sizing: content-box; | |||
There was a problem hiding this comment.
Could you remove these comments?
| bottom: -$carousel-indicator-hit-area-height; | ||
| left: -$carousel-indicator-hit-area-height / 2; | ||
| display: block; | ||
| height: $carousel-indicator-hit-area-height * 3; |
There was a problem hiding this comment.
Height is redundant since we already set top and bottom
| $carousel-indicator-transition: opacity .6s ease !default; | ||
|
|
||
| $carousel-caption-width: 70% !default; | ||
| $carousel-caption-color: $white !default; | ||
| $carousel-caption-color: inherit !default; |
| border-top: $carousel-indicator-hit-area-height solid transparent; | ||
| border-bottom: $carousel-indicator-hit-area-height solid transparent; | ||
| // border-top: $carousel-indicator-hit-area-height solid transparent; | ||
| // border-bottom: $carousel-indicator-hit-area-height solid transparent; | ||
| opacity: .5; |
There was a problem hiding this comment.
Probably interesting to make this a variable
| @@ -158,21 +161,35 @@ | |||
| list-style: none; | |||
|
|
|||
| li { | |||
| box-sizing: content-box; | |||
| position: relative; | |||
| // box-sizing: content-box; | |||
| flex: 0 1 auto; | |||
| width: $carousel-indicator-width; | |||
| height: $carousel-indicator-height; | |||
| margin-right: $carousel-indicator-spacer; | |||
| margin-left: $carousel-indicator-spacer; | |||
| text-indent: -999px; | |||
There was a problem hiding this comment.
Do we really need this? No text is added in the thumbnails anyway. If it needs text, we can work with aria-labels?
| $carousel-control-color: $white !default; | ||
| $carousel-color: $white !default; | ||
|
|
||
| $carousel-control-color: inherit !default; |
| @@ -13,6 +13,7 @@ | |||
|
|
|||
| .carousel { | |||
| position: relative; | |||
| color: $carousel-color; | |||
There was a problem hiding this comment.
I like this. Maybe we can also add an example with light slides with a .text-dark added to the carousel?
| </div> | ||
| </div> | ||
| </div> | ||
| <a class="carousel-control-prev" href="#carouselExampleColors" role="button" data-slide="prev"> |
There was a problem hiding this comment.
<a role="button">, looks nasty. Can we just use <button>s here?
There was a problem hiding this comment.
This is how the carousel works today, so this isn't any kind of regression. I'm down to use buttons here, but I'd need help to rewrite the JS that attaches these anchors to the appropriate carousel.
|
Closing as stale. |
WIP, do not merge.
This PR starts some work to revamp the carousel to use
currentColor, allowing us to better use utilities to change text, control, and indicator colors. Can't recall where I left this off previously, so need to dig back in before I get a checklist of todos going. At the very least I'll need to consider some migration docs.