DRAFT: carousel revamp #4635
DRAFT: carousel revamp #4635
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Fercas123
force-pushed
the
revamp-carousel
branch
from
be6e19b to
da3e255
Compare
| */ | ||
| id?: string; | ||
| bordered?: boolean; |
There was a problem hiding this comment.
Styles might need to be checked, currently this shifts the alignment
There was a problem hiding this comment.
that's intentional, but the prop name might not give it very clearly... I will check this with @ivan-calderon but there is a Hero (transparent) version and a...norther(bordered) one hahaha will check the naming
| const ControlsLabel = () => ( | ||
| <Text as="span"> | ||
| <strong> | ||
| {/* TODO: check w dev, this will need changing or a formatter*/} |
There was a problem hiding this comment.
Probably fine for now, we should align with Pagination
There was a problem hiding this comment.
you mean secondary & no bold? pagination seem to be forcing the of in, no formatter... should there be one to allow users to format the info?
Fercas123
force-pushed
the
revamp-carousel
branch
from
db6eb61 to
0cc5ed8
Compare
|
/release-pr |
There was a problem hiding this comment.
A few additional items:
-
When the previous or next button is pressed the position should also be conveyed to screen reader users (e.g. "slide label" 2 of 5)
-
When previous button is pressed moving the carousel back to slide 1 (if we are not allowing wrapping) and the button becomes disabled, focus should be placed on the next button. Similarly when pressing next button for the last slide, when the next button is disabled, focus should be placed on the previous button.
-
Just checking, for mobile view/small viewport did we want to leave the previous/next buttons at the top?
| <Carousel | ||
| {...args} | ||
| visibleSlides={{ sm: 1, md: 2 }} | ||
| aria-label="Categorical backgrounds carousel" |
There was a problem hiding this comment.
@Fercas123 it is likely just because it is an example but I recommend removing "carousel" from the aria label as aria-roledescription already indicates it is a carousel so it is repeated twice.
| {Media && <div className={withBaseName("mediaContainer")}>{Media}</div>} | ||
| <div className={withBaseName("fixedContainer")} ref={buttonBarRef}> | ||
| <div | ||
| role="group" |
There was a problem hiding this comment.
Each slide container with role group should also have aria-roledescription set to "slide".
In addition each slide should have an accessible name:
-
If a slide has a visible label, its accessible label is provided by the property aria-labelledby on the slide container set to the ID of the element containing the visible label (e.g. the h3).
-
Otherwise, an accessible label is provided by the property aria-label set on the slide container.
-
If unique names that identify the slide content are not available, a number and set size can serve as a meaningful alternative, e.g., "3 of 10". Note: Normally, including set position and size information in an accessible name is not appropriate. An exception is helpful in this implementation because group elements do not support aria-setsize or aria-posinset.
-
Note that since the aria-roledescription is set to "slide", the label should not contain the word "slide."
| ref={useForkRef(ref, containerRef)} | ||
| className={withBaseName()} | ||
| aria-live="polite" | ||
| role="region" |
There was a problem hiding this comment.
We only need 1 role="region", it looks like we have one on the carousel container that has aria-roledescription="carousel" which includes all the elements. We can remove this one.
| <div | ||
| ref={useForkRef(ref, containerRef)} | ||
| className={withBaseName()} | ||
| aria-live="polite" |
There was a problem hiding this comment.
While it is optional, I recommend adding aria-atomic set to false
Reference: APG carousel
| className={withBaseName()} | ||
| aria-live="polite" | ||
| role="region" | ||
| tabIndex={0} |
There was a problem hiding this comment.
If we want to make the slide focusable, can we move the tabindex="0" to the slide container has role group.
No description provided.