amp-carousel: optional aria-label for navigation buttons #9431
Conversation
aria-labels on the 'back' and 'next' buttons of the carousel can now be changed instead of defaulting to 'next/previous item in carousel'
This reverts commit 91a75a9.
closes Issue ampproject#422
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
Following the JS Style Guide
(forreal this time, they are really gone)
There was a problem hiding this comment.
Looks pretty good Alanna. A few requests:
- Couple of nit comments in the code
- Let's also rename the title of the PR to include the component it is changing,
e.gamp-carousel: optiona aria-label for navigation buttons - Let's document these new attributes in the
.mdfile of the component.
examples/carousel.amp.html
Outdated
| @@ -98,7 +98,7 @@ | |||
|
|
|||
| </amp-carousel> | |||
|
|
|||
| <amp-carousel width=auto height=300 controls> | |||
| <amp-carousel width=auto height=300 data-next-button-aria-label="Go Forward" data-previous-button-aria-label="Go Back" controls> | |||
There was a problem hiding this comment.
nit: Let's use more descriptive values here to set a good best practice precedence. Maybe
Go to next slide and Go to previous slide
| @@ -71,7 +71,13 @@ export class BaseCarousel extends AMP.BaseElement { | |||
| this.prevButton_.setAttribute('role', 'button'); | |||
| // TODO(erwinm): Does label need i18n support in the future? or provide | |||
| // a way to be overridden. | |||
| this.prevButton_.setAttribute('aria-label', 'Previous item in carousel'); | |||
| if (this.element.hasAttribute('data-previous-button-aria-label')) { | |||
There was a problem hiding this comment.
remove the // TODO above since it is done
alannawalton
changed the title
Updated aria-labels to be more descriptive. Removed unnecessary TODOs. Altered title of pull request. Updated .md to describe feature
|
@alannawalton Looks great. I think you need to add your |
|
I signed it |
|
CLAs look good, thanks! |
|
Merged! |
With these new attributes 'data-next-button-aria-label' and 'data-previous-button-aria-label' the aria-label can be set for each of these elements.
Closes #442