Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added aria label to the button block with icon #38966

Merged
merged 6 commits into from
Feb 25, 2022
Merged

Added aria label to the button block with icon #38966

merged 6 commits into from
Feb 25, 2022

Conversation

Sisanu
Copy link
Contributor

@Sisanu Sisanu commented Feb 21, 2022 •
edited
Loading

Description

Testing Instructions

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@Sisanu Sisanu requested a review from ajitbohra as a code owner February 21, 2022 18:45
@Sisanu Sisanu mentioned this pull request Feb 21, 2022
Merged
8 tasks
alexstine
alexstine reviewed Feb 21, 2022
View reviewed changes
Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sisanu Thanks for the PR. Please see my comment below. Otherwise, looking great.

alexstine
alexstine reviewed Feb 22, 2022
View reviewed changes
packages/block-library/src/search/edit.js Outdated
@@ -257,6 +257,7 @@ export default function SearchEdit( {
type="button"
className={ buttonClasses }
style={ buttonStyles }
aria-label={ label ? label : __( 'Search' ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sisanu Probably going to want to make this buttonLabel. Need to figure out how to remove all the HTML tags though. Not sure if we have package for it or not.

import { __unstableStripHTML } from '@wordpress/dom';

That may be safe to use. I saw another PR get merged using it. Still I am unsure why it is marked unstable.

Copy link
Contributor Author

@Sisanu Sisanu Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexstine, are you aiming for something like this? As far as I understand we have a buttonText attribute.
aria-label={ buttonText ? stripHTML( buttonText ) : __( 'Search' ) }

https://capture.dropbox.com/B5BduQpKH0HJy4Fe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sisanu Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed, please review.

alexstine
alexstine approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sisanu looks good.

@talldan can you give this a quick look? I am confident this is done well but would like to get a review on the unstable strip HTML.

@talldan
Copy link
Contributor

talldan commented Feb 24, 2022

Thanks for working on this!

The only thing I'd mention is that we're using the label attribute in PHP for the front-end of a site (as changed in #38136) and the buttonText attribute in JavaScript for the editor.

I think it'd be good if the same attribute could be used in both places.

@alexstine
Copy link
Contributor

@talldan Nice catch. I totally thought we were using buttonText in that other PR, but guess not. This is super confusing. Should buttonText be used in both places since it is an aria-label for the button? If not, I am sure one quick commit can switch back to label for this PR.

Sorry for the confusion, I just don't understand very well how this side of the editor works and tries to tie in with old WordPress search.

@talldan talldan added [Block] Buttons Affects the Buttons Block [Type] Bug An existing feature does not function as intended labels Feb 25, 2022
talldan
talldan approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Sisanu - the changes look good now.

@talldan talldan merged commit 9c5549e into WordPress:trunk Feb 25, 2022
@talldan talldan changed the title Added aria label to the button with icon Added aria label to the button block with icon Feb 25, 2022
@github-actions github-actions bot added this to the Gutenberg 12.8 milestone Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talldan talldan talldan approved these changes

@alexstine alexstine alexstine approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees
No one assigned
Labels
[Block] Buttons Affects the Buttons Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 12.8
Development

Successfully merging this pull request may close these issues.
3 participants
@Sisanu @talldan @alexstine