-
Notifications
You must be signed in to change notification settings - Fork 47
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
#2294 Fix Accessibility Violations - 2 #2295
#2294 Fix Accessibility Violations - 2 #2295
Conversation
Signed-off-by: srikant <[email protected]>
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.
Does carbon have this icon? We shouldn't need to include most icons as this point.
Signed-off-by: srikant <[email protected]>
I have updated |
Can we delete the replaced icons? |
Signed-off-by: srikant <[email protected]>
@tomlyn can you review this? |
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.
Hi Srikant
While I think it's a good idea to replace the icons with those from the Carbon Icons Library this change has unfortunately removed some function.
That is, in the current code when the notification panel contains messages, the 'dot' in the notification counter icon is colored to indicate the top-most severity of the messages shown in the panel.
You can test this in the test harness by adding a message using the API right flyout. When you add a message you can set its severity ("message type") and the dot will be colored based on which is the most severe message. Like this:
Unless we get approval from Design to remove this function we probably ought to keep it working the same way.
…ing role and aria-label Signed-off-by: srikant <[email protected]>
Signed-off-by: srikant <[email protected]>
Signed-off-by: srikant <[email protected]>
@tomlyn I have reverted that Also included |
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.
LGTM
Ref: #2294
Fixes Notification Icon Accessibility Violation.
Fixes Properties Table Icon Accessibility Violations.
Developer's Certificate of Origin 1.1