Fix/issue 11669 block icon #12554
Fix/issue 11669 block icon #12554
Conversation
There was a problem hiding this comment.
Hi @timwright12, thank you for your contribution I left some suggestions regarding the code.
From the functional and design point of view I think things look as expected only the opacity was removed:
| className="editor-block-switcher__no-switcher-icon" | ||
| label={ __( 'Block icon' ) } | ||
| > | ||
| <div className="components-toolbar" aria-hidden="true"> |
There was a problem hiding this comment.
We are using the class of a component outside of it in another element. I guess in this case we should use a different class and add the minimum necessary styles to have a similar UI.
There was a problem hiding this comment.
As mentioned this should be:
| <div className="components-toolbar" aria-hidden="true"> | |
| <div className="components-block-switcher" aria-hidden="true"> |
As mentioned by Jorge you'll need to duplicate some styles from components-toolbar, I'd imagine, to get this to work.
There was a problem hiding this comment.
@jorgefilipecosta @tofumatt All set here
| label={ __( 'Block icon' ) } | ||
| > | ||
| <div className="components-toolbar" aria-hidden="true"> | ||
| <span className="components-icon-button editor-block-switcher__no-switcher-icon is-presentational"> |
There was a problem hiding this comment.
Same as in the toolbar case we should avoid the usage of components-icon-button class as it is internal to another component in another package.
There was a problem hiding this comment.
@jorgefilipecosta @tofumatt All set here as well. Let me know if you find anything else. Thanks for the feedback!
jorgefilipecosta
requested review from
karmatosed,
tofumatt,
jasmussen and
afercia
jorgefilipecosta
added
[Focus] Accessibility (a11y)
| className="editor-block-switcher__no-switcher-icon" | ||
| label={ __( 'Block icon' ) } | ||
| > | ||
| <div className="components-toolbar" aria-hidden="true"> |
There was a problem hiding this comment.
As mentioned this should be:
| <div className="components-toolbar" aria-hidden="true"> | |
| <div className="components-block-switcher" aria-hidden="true"> |
As mentioned by Jorge you'll need to duplicate some styles from components-toolbar, I'd imagine, to get this to work.
|
@jorgefilipecosta hi, was going through the issues with the "Needs accessibility feedback" label: what kind of feedback is needed here? |
Hi @afercia, I guess we just need to compare with the previous version and check if the main objective was accomplished: The disable block icon is semantically presentational and is removed from assistive technologies. |
| .components-icon-button.editor-block-switcher__toggle, | ||
| .components-icon-button.editor-block-switcher__no-switcher-icon { | ||
| margin: 0; | ||
| .components-icon-button.editor-block-switcher__no-switcher-icon, |
There was a problem hiding this comment.
.editor-block-switcher__no-switcher-icon class is getting removed so should be removed from this file, too.
| className="editor-block-switcher__no-switcher-icon" | ||
| label={ __( 'Block icon' ) } | ||
| > | ||
| <div className="components-block-switcher" aria-hidden="true"> |
There was a problem hiding this comment.
I don't think, we should be re-creating Toolbar here as this is still the same div.
timwright12
requested review from
noisysocks,
talldan and
youknowriad
as code owners
|
From an accessibility perspective, looks good to me. |
|
Looks like this has been fixed with #13721 so I'm closing this PR. If I missed anything we can reopen if needed. |
Description
Fixes #11669
How has this been tested?
Types of changes
Converted disabled block icon buttons to a span so can can be semantically presentational and removed from assistive technology.
Checklist: