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

Rating widget accessibility #281

Closed
wants to merge 3 commits into from
Closed

Conversation

smhigley
Copy link

Updates the rating widget to use radio inputs for each star to add proper semantics and keyboard accessibility.

Resolves #280

Copy link
Member

@dylans dylans left a comment

Choose a reason for hiding this comment

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

one quick question on one of the comments.

form/Rating.js Outdated
_onClick: function(evt) {
if (evt.target.tagName === 'LABEL') {
var clickedValue = +domAttr.get(evt.target.querySelector('input'), "value");
// for backwards compatibility
Copy link
Member

Choose a reason for hiding this comment

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

backwards compatibility with previous versions of Dojo or browsers or ?

Copy link
Author

@smhigley smhigley Apr 16, 2018

Choose a reason for hiding this comment

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

For previous versions of Dojo, because there used to be a value attribute directly on the <li> tag. Added this in case anyone was listening to the click event and looking at evt.target.value.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, the comment should say that so we remember what type of compatibility we were trying to preserve. :)

@dylans dylans added this to the 1.13.1 milestone Apr 16, 2018
@dylans
Copy link
Member

dylans commented Apr 17, 2018

Thanks @smhigley!

Closed via 8b7a975. Backported as b454122 (1.13), ee5ee6c (1.12), bdca832 (1.11), 592f137 (1.10), 6967e62 (1.9), and a6a4666 (1.8).

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.

2 participants