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

[Slider] Add getAriaLabel prop #17240

Merged
merged 3 commits into from
Sep 2, 2019
Merged

[Slider] Add getAriaLabel prop #17240

merged 3 commits into from
Sep 2, 2019

Conversation

city41
Copy link
Contributor

@city41 city41 commented Aug 30, 2019

Fixes #17231

I followed the pattern set by getAriaValueText. I noticed its documentation wasn't quite right (missed @returns {string} so I added that to both functions.

The only test I added is for the console warning, which matches the test coverage of getAriaValueText.

Also, should aria-labelledby have an equivalent getAriaLabelledBy?

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 30, 2019

Details of bundle changes.

Comparing: f55f3a4...76b3746

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.01% 🔺 330,879 330,921 90,349 90,361
@material-ui/core/Paper 0.00% 0.00% 68,774 68,774 20,485 20,485
@material-ui/core/Paper.esm 0.00% 0.00% 62,148 62,148 19,214 19,214
@material-ui/core/Popper 0.00% 0.00% 28,466 28,466 10,190 10,190
@material-ui/core/Textarea 0.00% 0.00% 5,094 5,094 2,137 2,137
@material-ui/core/TrapFocus 0.00% 0.00% 3,834 3,834 1,613 1,613
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,385 16,385 5,827 5,827
@material-ui/core/useMediaQuery 0.00% 0.00% 2,541 2,541 1,058 1,058
@material-ui/lab +0.03% 🔺 +0.05% 🔺 153,137 153,179 46,644 46,669
@material-ui/styles 0.00% 0.00% 51,492 51,492 15,303 15,303
@material-ui/system 0.00% 0.00% 15,668 15,668 4,359 4,359
Button 0.00% 0.00% 78,661 78,661 24,053 24,053
Modal 0.00% 0.00% 14,344 14,344 5,019 5,019
Portal 0.00% 0.00% 2,907 2,907 1,318 1,318
Rating 0.00% 0.00% 70,014 70,014 21,866 21,866
Slider +0.06% 🔺 +0.08% 🔺 74,240 74,282 22,994 23,012
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 52,253 52,253 13,780 13,780
docs.main 0.00% 0.00% 597,355 597,355 190,798 190,798
packages/material-ui/build/umd/material-ui.production.min.js +0.02% 🔺 +0.04% 🔺 301,797 301,843 86,659 86,692

Generated by 🚫 dangerJS against 76b3746

@oliviertassinari oliviertassinari added component: slider This is the name of the generic UI component, not the React module! new feature New feature or request labels Aug 30, 2019
@oliviertassinari oliviertassinari changed the title [Slider] add getAriaLabel [Slider] Add getAriaLabel prop Aug 30, 2019
@city41
Copy link
Contributor Author

city41 commented Aug 30, 2019

renamed the commit to make the new PR name

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

This looks great! Should we update http://material-ui.com/components/slider/#accessibility too? Like

diff --git a/docs/src/pages/components/slider/slider.md b/docs/src/pages/components/slider/slider.md
index c85ada73f..bc9d5a4a0 100644
--- a/docs/src/pages/components/slider/slider.md
+++ b/docs/src/pages/components/slider/slider.md
@@ -53,7 +53,7 @@ Continuous sliders allow users to select a value along a subjective range.
 The component handles most of the work necessary to make it accessible.
 However, you need to make sure that:

-- The slider, as a whole, has a label (`aria-label` or `aria-labelledby` prop).
-- Each thumb has a user-friendly name for its current value.
+- Each thumb has a user-friendly label (`aria-label`, `aria-labelledby` or `getAriaLabel` prop).
+- Each thumb has a user-friendly text for its current value.
 This is not required if the value matches the semantics of the label.
 You can change the name with the `getAriaValueText` or `aria-valuetext` prop.

packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

Don't worry about the commit name, we erase it when we merge.

@mbrookes
Copy link
Member

While we're here, should we @ignore the aria-* props? As a rule we don't document standard props in the API documentation.

@oliviertassinari
Copy link
Member

I think that we should keep it because we have a non default behavior, the aria prop is not applied on the root element.

@mbrookes
Copy link
Member

the aria prop is not applied on the root element

The API docs don't mention that, they simply document the standard aria props. If that implementation detail is important, it could be mentioned in the accessibility section.

Speaking of which, what does "This is not required if the value matches the semantics of the label." mean?

@city41
Copy link
Contributor Author

city41 commented Aug 31, 2019

I updated the messages as well as slider.md's accessibility section (didn't realize that existed, thanks for the heads up)

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This is a great step towards better a11y of range sliders.

Could you add a test with a range slider and getAriaLAbel? I think you can use getByLabelText() and check that it has the slider role. That should cover most a11y concerns of this feature.

packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added accessibility a11y and removed new feature New feature or request labels Sep 1, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick about test failure messages.

@oliviertassinari oliviertassinari merged commit dc07446 into mui:master Sep 2, 2019
@oliviertassinari
Copy link
Member

Awesome

@city41 city41 deleted the sliderGetAriaLabel branch September 3, 2019 15:19
@angel-langdon
Copy link

getAriaLabel doesn't work.
I am applying this function

labeltext(index) {
   console.log(this.state.data[index])
  return this.state.data[index]; // "string"
}

but it seems getAriaLabel isn't even being called

@eps1lon
Copy link
Member

eps1lon commented Aug 24, 2020

@007sya Please open a new issue and fill out the template. It's hard to to resolve in an already merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slider: should be able to label each thumb with a different aria-label
6 participants