-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Make carousel indicators actual buttons #32661
Conversation
ec39752
to
e6cf4e8
Compare
e6cf4e8
to
dee29d4
Compare
This relies on #32631 being merged to remove the overridden focus indicator for the button in reboot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this is really needed.
I only wonder if we shouldn't introduce anew class name for single indicator (eg .carousel-indicator
) instead of relying on data-
attribute?
Edit: would be a breaking change.
yup, exactly. i see this as a first step, then we can make it a breaking change next time to offer more styling flexibility |
@mdo good to merge? |
Nope. These are added to beta3.
…On Sun, Jan 17, 2021, 14:22 Patrick H. Lauke ***@***.***> wrote:
@mdo <https://github.com/mdo> good to merge?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#32661 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNNI4O5GB7R3J3XGAULS2LJADANCNFSM4VRZCBLA>
.
|
So, about the JS part. Before, we were only looping once. Now we loop twice. Why can't we use |
39e3e47
to
be40c85
Compare
because it's not guaranteed that they're immediate children. before, with our structure, it was just a list, and the indicators were the immediate i.e. I could decide that yes, I still want an ordered list structure, so end up with
or whatever. |
How about closest then?
…On Thu, Jan 28, 2021, 22:03 Patrick H. Lauke ***@***.***> wrote:
Why can't we use children() like before
because it's not guaranteed that they're immediate children. before, with
our structure, it was just a list, and the indicators were the immediate
<li> elements. now, you may be using one container for all the
indicators, and then have a few more structures in between, and only
*then* the links or buttons of whatever you're using.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#32661 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNNCIFWGYFIBEY5ITZLS4G7KHANCNFSM4VRZCBLA>
.
|
NVM, that probably wouldn't work either. I guess the current solution is as
good as it gets, then. I'll review tomorrow and merge it.
…On Thu, Jan 28, 2021, 22:04 XhmikosR ***@***.***> wrote:
How about closest then?
On Thu, Jan 28, 2021, 22:03 Patrick H. Lauke ***@***.***>
wrote:
> Why can't we use children() like before
>
> because it's not guaranteed that they're immediate children. before, with
> our structure, it was just a list, and the indicators were the immediate
> <li> elements. now, you may be using one container for all the
> indicators, and then have a few more structures in between, and only
> *then* the links or buttons of whatever you're using.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#32661 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AACVLNNCIFWGYFIBEY5ITZLS4G7KHANCNFSM4VRZCBLA>
> .
>
|
I don't think my code looks for more than one? I do |
Make carousel indicators actual buttons
Make carousel indicators actual buttons
- In Bootstrap 5 the CSS selector matching indicators has changed and included `data-bs-target` attribute that we were missing - Recommended carousel markup changed from using `ul`, `li` and `a` to using `button` elements - Had to refactor tests as selectors were not precise enough On Bootstrap side see: - twbs/bootstrap#32661 - twbs/bootstrap#32627 Fixes ng-bootstrap#4200 Fixes ng-bootstrap#4253
- In Bootstrap 5 the CSS selector matching indicators has changed and included `data-bs-target` attribute that we were missing - Recommended carousel markup changed from using `ul`, `li` and `a` to using `button` elements - Had to refactor tests as selectors were not precise enough On Bootstrap side see: - twbs/bootstrap#32661 - twbs/bootstrap#32627 Fixes #4200 Fixes #4253
Currently, the carousel indicators are not conveyed at all to assistive technologies, and they can only be operated using the mouse.
This change generalises some of the stylings and JS to be backwards-compatible (meaning it's non-breaking), but caters for a more correct structure where the indicators are actual button elements (thus keyboard-focusable/operable), and where the currently active indicator is conveyed with
aria-current
Preview: https://deploy-preview-32661--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/#with-indicators