Skip to content
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

Issue 266 #352

Merged
merged 3 commits into from Oct 8, 2019
Merged

Issue 266 #352

merged 3 commits into from Oct 8, 2019

Conversation

guily19
Copy link

@guily19 guily19 commented Aug 7, 2019

Fixed the accessibility error for the next and previous buttons

-Add the aria-label property to fix the accesibility error of previous and next buttons
- Update the snapshot to fullfill the new conditions
@leandrowd
Copy link
Owner

Great stuff @guily19 , thanks for the contribution! I have a few thoughts and would like your opinion.

  1. Do you think we need labels for other controls too? If our goal is accessibility I would say yes. If the goal is just to pass a given validation, maybe. I would prefer to focus on the former.
  2. Given that we might need more strings being passed down, I believe we could group all of them in a single object passed as a prop. (i.e: prop named labels with attributes like "leftArrow", "rightArrow", "controls", "item", etc)

Please let me know what you think.

@MattBridgeman
Copy link

MattBridgeman commented Aug 13, 2019

@leandrowd @guily19

  1. I've been working with @guily19 on this and our use-case is passing accessibility tool validation but if you want more labels elsewhere we can look into that. If so could you list what you want labeled? I would suggest the ones we want to add labels to are:
  • Thumbnails left and right buttons
  • Thumbnail item
  • Carousel "Control dots"
  1. Makes sense we can work on that

@MattBridgeman
Copy link

@leandrowd @guily19
Okay following you're comments I've done the following:

  1. Do you think we need labels for other controls too?
    a. Labeled all previous and next buttons both in the carousel and thumb components
    b. labeled all slide item buttons in the carousel and thumb components
  2. I believe we could group all of them in a single object passed as a prop
    a. I've created a labels object in props with a default value for "leftArrow", "rightArrow" and "item"

I've updated the readme too. Any other comments please let us know @leandrowd

@afnizarnur
Copy link

Any updates with this? waiting for this PR to be merged 😬

@leandrowd leandrowd merged commit 9ce4bf6 into leandrowd:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants