Always show icon in block toolbar #11600
Conversation
jorgefilipecosta
added
[Type] Task
|
You beat me! 😂 |
|
Screenshots:
Generally, I dig it! This works. Can we make sure that the icon button is always as wide as it would be, if it was a switcher? I think we may need to set a fixed width on both, though I think the one that has a transformation maybe already has a transform. Should we also try and dimming the opacity to .8 to imply it's disabled? CC: @alexislloyd for thoughts. |
|
Let's try the 0.8 opacity to address @youknowriad's comment on #11552 and see what that looks like?
|
jorgefilipecosta
force-pushed
the
update/always-show-block-icon-on-the-switcher
branch
from
5dd712e
to
dc6efd3
Compare
|
Should we also include this icon when editing as HTML? It should be a separate PR if that makes sense. We would also need to add This is how the same Vimeo block looks when editing as HTML: |
|
Hi @jasmussen and @alexislloyd thank you for the reviews the feedback was applied and the width and opacity were changed: |
|
I dig that the icon box is now the same width as the switcher. I'm wondering if the .8 opacity isn't too little. For actually disabled buttons, we go as far as 0.3:
But I'm not sure if that will look extra broken, or whether that is actually helpful. |
jorgefilipecosta
force-pushed
the
update/always-show-block-icon-on-the-switcher
branch
from
dc6efd3
to
8ed3401
Compare
|
Hi @jasmussen we are now using the default opacity value for disabled buttons 0.3:
I think this looks better and makes it easy to understand the button is disabled. |
|
I think I agree, that makes sense. But I will defer to @mtias and @alexislloyd as I know they have thoughts on this. |
|
I don't think the opacity should be adjusted at all. For starters it almost definitely won't past a contrast test, and even though the button is disabled it still needs to be usable as an indication of the block's type. At that opacity it also looks like something is potentially wrong with the block, or something isn't fully loaded or something. It just feels like something is off. If we continue showing the dropdown arrow, maybe there's an argument to be made for the opacity change, but as we're already hiding the arrow I don't feel like the opacity change adds anything, and in fact is detrimental. |
|
@chrisvanpatten How do you address this point
|
|
Can we get this in and open a follow-up PR if we find a better way to signal that this icon is non-actionable? |
|
It's worth noting that if a button truly is disabled, it doesn't have to meet contrast ratio requirements. In fact it needs to be visibly disabled. This is a gray area, admittedly, because it's not a button that's disabled. But it more or less acts as the same, and the distinction is largely intellectual. |
|
For the sake of iteration and releasing 4.3 I'm merging but please let the discussion continue and improve on what we have if we find a better alternative. |
| @@ -58,7 +58,20 @@ export class BlockSwitcher extends Component { | |||
| const hasStyles = blocks.length === 1 && get( blockType, [ 'styles' ], [] ).length !== 0; | |||
|
|
|||
| if ( ! hasStyles && ! possibleBlockTransformations.length ) { | |||
| return null; | |||
| if ( blocks.length > 1 ) { | |||
There was a problem hiding this comment.
Why do we care about whether there's a multi-selection here, when in the rendering of the IconButton below this outer if block, we use blockType.icon (the first selected block's icon) whether or not it's a multi-selection.
Arrived here via rebasing of #12828, where this code is problematic and difficult to resolve due to the inconsistent forking logic.
Closes: #11552
This PR applies a change to the block switcher now when a single block is selected it renders the block icon disabled and without the arrow or hover effect as discussed in #11552.
How has this been tested?
Verify that the icon of the columns block or "Media & Text" correctly appears.
Screenshots