Rating: Button Controlled Example should announce itself on Narrator and unchecked stars should meet color contrast ratio. #7955
Conversation
|
|
@betrue-final-final Can you look at the screener changes? I believe the updates need to be looked at for approval. |
aneeshack4
changed the title
|
Just wanted to update - neutralSecondaryAlt is about to be deprecated so we're trying to see if hollow stars can just replace them altogether. |
Interesting... hollow stars will then result in higher ratings for the control right? Curious to know answers to the following since I haven't been part of such a design change. How should we communicate this change to partners? As a minor or major release? cc: @Vitalius1 |
There was a problem hiding this comment.
Don't use the deprecated color. We need @betrue-final-final to review and make a decision about what should be used here.
| @@ -38,7 +38,7 @@ export function getStyles(props: IRatingStyleProps): IRatingStyles { | |||
| const ratingVerticalPadding = 8; | |||
| const ratingHorizontalPadding = 2; | |||
|
|
|||
| const ratingStarUncheckedColor = palette.neutralTertiary; | |||
| const ratingStarUncheckedColor = palette.neutralSecondaryAlt; | |||
There was a problem hiding this comment.
Definitely not use this color because there is an open issue for removing the use of this color: #6575
@KevinTCoughlin not sure what you meant by this... how they will result in higher ratings? As for whether this is a major or minor change, not really sure what to tell. From a logic perspective, we are not touching anything so it should be a patch type. Maximum I can think of it being a minor type and leave a very descriptive release note of why this has been done. We would not break any layouts if we would go with replacing them with hollow stars so should be on a safe side... but will leave @betrue-final-final to decide 😄 |
|
@betrue-final-final - gentle ping here, can you give guidance to what to do here? |
@Vitalius1, sorry I just now realized how my phrasing is confusing given the component we're discussing :). I meant, "Will using a hollow star result in higher accessibility ratings i.e. AAA?"
I can imagine consumers being surprised by a patch update changing the styling of this control in their app and also if it were a minor update. I'm unsure if we have a pattern for this type of design change with regards to our SemVer cc: @micahgodbolt |
|
FYI, here's the suggested design, which uses shape, and not just color to pass contrast between states as well as against the background. |
|
S-M hover is incorrect and should be hollow. Fixing that |
|
@micahgodbolt Any decision reached on this design? If we are going ahead with this design, I believe @betrue-final-final has it implemeneted locally in a branch? Just want to get this merged soon because it's been around for a while and it's the only thing blocking me from resolving the Rating A11y Pass. |
|
Seems like this needs further discussion so I'm going to soft close and reopen once we decide on the right design. |
|
@betrue-final-final @Vitalius1 Could we use this color in the meantime while the hollow stars are implemented? It's blocking this PR from being merged and that's blocking the Rating A11y pass from being resolved. |
|
If we use this color, then Rating still really isn't passing accessibility, merely fixing this one characteristic of it. I just want to be clear that we are solving the issue of contrast against its background, but not between checked and unchecked stars. |
|
The title of this issue is far too general. |
|
I'd rather get this done right than just try to get our A11y pass finished. If we need to add some hollow star icons to our subset, we can do that. |
aneeshack4
changed the title
|
Changed the title. I will add the hollow star icons to our subset and use those to hopefully fix the color contrast issue. |
Pull request checklist
$ npm run changeDescription of changes
Bug 2: fixed color contrast ratio for unchecked Rating stars
Changing the color to neutralSecondaryAlt for when rating stars are unchecked so that it meets contrast ratio to 4.5:1
Small text at level aaa is the only one that fails but text will never be overlayed on these unchecked stars and I tried a bunch of the other colors in DefaultPalette.ts that would make sense in this situation and the only other one that fully passed was neutralPrimaryAlt but it's super dark to the point where unchecked state is indistinguishable from checked state:
Bug 4: fixed Button Controlled Rating example so that it announces itself through Narrator
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow