Accessibility: Support aria-labels #125
Conversation
|
Thanks @atlidohop! Looks good to me. Just one nitpick - what's the purpose of making aria props default to empty string? Shouldn't they stay |
|
Good point, @mpowaga . Changes coming up. |
|
Beware, this is untested code, but works for my use case. How can I run index.html? |
…sible for contributing developers
|
This should work for all cases now @mpowaga . I also made some changes to make this package more accessible to contributing developers. Let me know if I'm breaking something somewhere else. 😅 |
|
I appreciate the effort to make it easier to contribute but React and other packages must be declared as |
|
Sure thing. I'll revert the changes in package.json. |
react-slider.js
Outdated
| @@ -734,6 +734,9 @@ | |||
| var className = this.props.handleClassName + ' ' + | |||
| (this.props.handleClassName + '-' + i) + ' ' + | |||
| (this.state.index === i ? this.props.handleActiveClassName : ''); | |||
| var ariaLabelArray = this.props.ariaLabel && this.props.ariaLabel.constructor === Array | |||
There was a problem hiding this comment.
At best we should use Array.isArray but because it's not supported by all browsers we need a polyfill. Can you add it at the top where we define our util functions, e.g.
var isArray = Array.isArray || function(x) {
return Object.prototype.toString.call(x) === '[object Array]';
};
react-slider.js
Outdated
| @@ -749,6 +752,10 @@ | |||
| "aria-valuenow": this.state.value[i], | |||
| "aria-valuemin": this.props.min, | |||
| "aria-valuemax": this.props.max, | |||
| "aria-label": ariaLabelArray && i < this.props.ariaLabel.length | |||
There was a problem hiding this comment.
Bound check is not necessary here (in fact it's an error because when out of bound access is detected it will pass an array to aria-label property). We can simply write it as follows.
"aria-label": isArray(this.props.ariaLabel) ? this.props.ariaLabel[i] : this.props.ariaLabel,There was a problem hiding this comment.
That's right! Changing this.
README.md
Outdated
| @@ -145,6 +145,16 @@ Callback called only after moving a handle has ended or when a new value is set | |||
|
|
|||
| Callback called when the the slider is clicked (handle or bars). Receives the value at the clicked position as argument. | |||
|
|
|||
| ##### ariaLabel {oneOfType([string, arrayOf(string)])} default: '' | |||
There was a problem hiding this comment.
default: '' can be removed now.
README.md
Outdated
| Use an array for more than one handle. | ||
| The length of the array must match the number of handles in the value array. | ||
|
|
||
| ##### ariaValuetext {string} default: '' |
There was a problem hiding this comment.
default: '' can be removed now (here as well 😄)
README.md
Outdated
| @@ -155,3 +165,18 @@ or an array of numbers in case of a multi slider. | |||
| ### License | |||
|
|
|||
| See the [License](LICENSE) file. | |||
|
|
|||
| ### Developers | |||
There was a problem hiding this comment.
We can remove this section for now and add it in another PR once we decide how development setup should work.
|
@mpowaga Thanks for good comments, much better now. 😄 |
Addresses issue #124