-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fix a11y problems on star rating component #2035
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Coming from Spec Meeting - May, 16:
|
…ange-OpenSource/Orange-Boosted-Bootstrap into main-mlh-fix-a11y-star-rating
This comment was marked as outdated.
This comment was marked as outdated.
Thanks for the review @Franco-Riccitelli But for me there is no reason to prevent a disabled star to be filled. For instance you may see the actual quotation but the component is disabled because you are not logged in. Or you don't have the right to change it. Or you changed the score but you remove a mandatory information so it become disabled. In fact I see many reasons why it can be possible to have checked stars in a disabled star rating component. This component is working like radio buttons and checkboxes so why should we reset it when it becomes disabled? If you are ok, I will need to know what color(s) you prefer for checked stars in a disabled star rating component please 😉 |
Ok, then we should use the same grey colour used for the outlines in the disabled state for the fill colour. |
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.
Few things more to tackle for the readonly and disabled variants
</fieldset> | ||
</form> | ||
{{< /example >}} | ||
|
||
### Readonly |
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 feels weird to have it here since it's not the same behavior and not the same purpose. Do you think we should have a proper section for this component (I mean the whole star rating) ?
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 could be a good thing yes since we now have normal/small versions + light/dark versions + disabled state + readonly state etc. It's probably too much for a simple section in the Checks & radios
page.
In addition the disabled
state is supposed to be used outside of a form, in a card for instance, so there is no reason to keep it in the Forms
section although the rest of the star rating thing is still a form element. 🤔
Should we split it? (🤢)
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 solution is working perfectly but because of the number of changes (adding a second See PR #2184 for alternative solution. |
Will be resolved by #2184 |
Related issues
Closes #2005
Closes #1764
Description
Fix a11y problems on star rating component.
Motivation & Context
Improve a11y.
Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)
After the merge