Improve Notice component accessibility. #54498
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
Mamaduka
reviewed
Sep 15, 2023
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
To avoid creating a fresh installation, we can just clear the preferences from the user meta:
wp user meta delete <user> wp_persisted_preferences
Contributor
Author
|
I think the Github actions are going a bit nuts?
|
Mamaduka
removed
the
[Type] Bug
Member
|
Sometimes, it fails when there's more than one P.S. I need to call it a day now, but I will properly test/review this on Monday if nobody beats me to it. |
Mamaduka
reviewed
Sep 15, 2023
| return __( 'Warning notice' ); | ||
| case 'info': | ||
| return __( 'Information notice' ); | ||
|
|
Member
There was a problem hiding this comment.
Nit: I think we can remove this empty gap here.
Contributor
|
Added to the components board so that sub-team is aware. 👍 |
Contributor
Author
This could be a problem, as Accessibility is typically something that spans across several areas and it is likely we'd need to att the Accessibility lable to issues and PRs that already have another [Type] label. |
Contributor
Author
|
There are still two things to fix here: 1 The button does already have a 2 
|
|
Flaky tests detected in 5812f03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6221279720
|
|
Size Change: -1.63 kB (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
aristath
approved these changes
Sep 18, 2023
Member
aristath
left a comment
aristath
left a comment
There was a problem hiding this comment.
I just added a small nitpick, but it;'s not a blocker so I'll go ahead and approve this 👍
Comment on lines
63
to
64
| case 'success': | ||
| default: |
Member
There was a problem hiding this comment.
Should we remove the case 'success': line here? The default already acts as a fallback, so that line doesn't serve a purpose 🤔
Suggested change
| case 'success': | |
| default: | |
| default: |
Contributor
Author
There was a problem hiding this comment.
I guss that for consistency the redundant case should be removed from getDefaultPoliteness as well.
ciampo
reviewed
Sep 19, 2023
Comment on lines
+112
to
121
|
|
||
| .components-notice__dismiss:not(:disabled):not([aria-disabled="true"]) { | ||
| color: $gray-400; | ||
| } | ||
|
|
||
| .components-notice__dismiss:not(:disabled):not([aria-disabled="true"]):focus, | ||
| .components-notice__dismiss:not(:disabled):not([aria-disabled="true"]):not(.is-secondary):active, | ||
| .components-notice__dismiss:not(:disabled):not([aria-disabled="true"]):not(.is-secondary):hover { | ||
| color: $gray-100; | ||
| color: $white; | ||
| } |
Contributor
There was a problem hiding this comment.
We should really avoid these styles overrides (and using internal components classnames outside of the components package), although I understand this is something that should be addressed separately from this PR.
ciampo
approved these changes
Sep 19, 2023
Member
Create a PR to fix this issue - #54724 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #54497
First pass at improving / fixing the Notice component accessibility. Please do not merge yet, needs design feedback.
What?
Imrpoves / fixes the Notice component accessibility.
Why?
There is no parity between the information provided visually and the information provided semantically / with text. As such, some users experience is less than ideal.
How?
Information noticetoCloseso that accessible name matches more closely the concept provided by the X icon.showTooltip={ false }so that the button renders its tooltip. Again worth reminding all icon buttons must visually expose their accessible name,Testing Instructions
Looking for template parts? Find them in "Patterns". notice is rendered.Information notice.Closeis rendered.Testing Instructions for Keyboard
Screenshots or screencast