Carousel: use buttons, not links, for prev/next controls #32627
Conversation
- expand the styles to neutralise border/background - change docs page - add extra unit test to check that links or buttons work as controls - modify visual test to use buttons as well
|
to clarify: using links still works, but it's not really semantically correct. not a breaking change, as the old markup still works. just better/cleaner to do it using |
|
temporarily set to draft while i expand this following merging of #32638 |
|
setting to draft to make sure the added carousel section (disabled touch) is added appropriately |
|
ah, my mistake, i see this won't be affected by my planned upcoming change. as you were |
|
There are more instances in examples? Also, perhaps we should have callout/example mentioning that both work but for whatever reasons we suggest buttons? |
- use buttons instead of links for prev/next - remove `role="button"` from links that are actually links
Ah, good catch, those slipped me by. Addressed now.
the "for whatever reason" part of your comment worries me a bit. i thought we (at least in core team) all understood/aknowledged that links are for linking (to another page, or a section within the current page like a skip anchor) and buttons are for dynamic in-page actions. i'll add a little mention/note about this though in a moment, sure. |
also adds code ticks to `id`
…strap into patrickhlauke-carousel
|
@patrickhlauke Is it intentional that the carousel controls sort of |
ah, good catch, hadn't even checked. no, wasn't intentional. forgot about Firefox's quirky behaviour of adding extra padding on |
|
Now, it looks good to me, thanks! One last comment and also I wonder, do we have an issue about this? |
we have this meta-issue where i reel off various things that should get fixed #31186 but no atomic issue about just this aspect i think. can do a trawl through open issues after this just to check though |
XhmikosR
changed the title
|
👍 thanks for sanity checking @XhmikosR |
Carousel: use buttons, not links, for prev/next controls
- 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
Preview https://deploy-preview-32627--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/