Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(accordion): use appropriate aria tags for accessibility - #5338

Closed
jeremytowne wants to merge 3 commits into
angular-ui:masterfrom
jeremytowne:accordion_a11y
Closed

feat(accordion): use appropriate aria tags for accessibility#5338
jeremytowne wants to merge 3 commits into
angular-ui:masterfrom
jeremytowne:accordion_a11y

Conversation

@jeremytowne

@jeremytowne jeremytowne commented Jan 25, 2016

Copy link
Copy Markdown

The id generation in accordion.js, seems a little sketchy - open to ideas about this.
@wesleycho

wesleycho commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

Something similar to

bootstrap/src/typeahead/typeahead.js

Line 113 in 3dac5e3

var popupId = 'typeahead-' + scope.$id + '-' + Math.floor(Math.random() * 10000);
would work I think for generating the id attribute - the Math.random usage is not the greatest solution, but is safe IMO.
@wesleycho wesleycho added this to the ARIA Accessibility milestone Jan 25, 2016
@jeremytowne

jeremytowne commented Jan 25, 2016

Copy link
Copy Markdown
Author

Ok, I will update it to be similar to that. Should I be adding the aria tags like

bootstrap/src/typeahead/typeahead.js

Line 114 in 3dac5e3

element.attr({
, or is putting them in the template OK?
@wesleycho

wesleycho commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

Template is fine
@jeremytowne

jeremytowne commented Jan 25, 2016

Copy link
Copy Markdown
Author

It looks like the travis ci build failed because of a timeout issue, is there anyway to run a new build against this?
@icfantv

icfantv commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Looks ok now.
wesleycho
wesleycho reviewed Jan 26, 2016
View reviewed changes
Comment thread src/accordion/accordion.js Outdated

@wesleycho wesleycho Jan 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you switch these to - instead of _?
@wesleycho

wesleycho commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Just that switch in those two ids to dashes & this should be good.
@jeremytowne

jeremytowne commented Jan 26, 2016

Copy link
Copy Markdown
Author

Awsome! Thanks for the feedback.
@wesleycho wesleycho modified the milestones: 1.1.2, ARIA Accessibility Jan 26, 2016
@wesleycho wesleycho closed this in 61cdef1 Jan 26, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.