-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
[material-ui][Rating] fix defaultLabelText a11y issue with undefine value input and hint #42810
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ZouYouShun. I left some small comments.
const _value = value || 0; | ||
return `${_value} Star${_value > 1 ? 's' : ''}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would give us:
- ❌
null
(or any falsy value) =>"0 Star"
(it should be in plural) - ❌
0
=>"0 Star"
(it should be in plural) - ✅
1
=>"1 Star"
- ✅
2
=>"2 Stars"
If we change it to this, we get better results:
const _value = value || 0; | |
return `${_value} Star${_value > 1 ? 's' : ''}`; | |
return `${value || "0"} Star${value !== 1 ? 's' : ''}`; |
- ✅
null
(or any falsy value) =>"0 Stars"
- ✅
0
=>"0 Stars"
- ✅
1
=>"1 Star"
- ✅
2
=>"2 Stars"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aarongarciah
LGTM, updated😎
* const _value = value || 0; | ||
* return `${_value} Star${_value > 1 ? 's' : ''}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modify this to match the previous comment in this file.
it('can be labelled with getLabelText when non set value', () => { | ||
render(<Rating readOnly />); | ||
|
||
expect(screen.getByRole('img')).toHaveAccessibleName('0 Heart'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be in plural.
expect(screen.getByRole('img')).toHaveAccessibleName('0 Heart'); | |
expect(screen.getByRole('img')).toHaveAccessibleName('0 Hearts'); |
@@ -225,6 +225,12 @@ describe('<Rating />', () => { | |||
expect(screen.getByRole('img')).toHaveAccessibleName('Stars: 2'); | |||
}); | |||
|
|||
it('can be labelled with getLabelText when non set value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test title seems incorrect.
it('can be labelled with getLabelText when non set value', () => { | |
it('should have a correct label when no value is set', () => { |
when I use Rating readOnly mode, I found there is a issue with the aria-label by default
defaultLabelText
which will render like below
aria-label="null Hearts"