Conditional Duotone Toolbar Visibilty

[ajlende]

Description

A block supporting duotone needs to be able to control the visibility of the toolbar. Data needs to be passed from the supporting block (i.e. image block) to the block supports (i.e. duotone).

See #31373

First off, the feature should not be visible in the setup state

Secondly, the Duotone button should always be at the end. Right now it starts as the first button, then if you deselect the block and select it again, it moves to the end

This change allows the image block to prevent rendering of the duotone toolbar. This has the side-effect of solving the second issue since the point where the duotone toolbar is rendered comes after the rest of the block toolbar items, so the toolbar button is now always rendered at the end.

How has this been tested?

1. Create a cover block
2. See that there isn't a duotone button
3. Add a background image
4. See that there is a duotone button
5. Enable parallax/repeat backgrounds or clear media
6. See that duotone button is hidden again
7. Repeat 1-4 for image block instead

Screenshots

Types of changes

New feature / Bug fix

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).

[jasmussen]

You should become a contributor to the repo so you don't have to work in a fork anymore ;)

This is what I see:

Looks good, it seems to fix the issue, and also seems to fix a flicker I noted in #31373.

What's the next step on this one? Does it just need a quick code review?

[ajlende]

You should become a contributor to the repo so you don't have to work in a fork anymore ;)

I started to on some of the newer PRs. My workflow was already set up for using my fork, so I just kept doing that. But I suppose it's a bit easier to check out and test a branch from here rather than my fork. :)

What's the next step on this one? Does it just need a quick code review?

I have it in draft because I'm still exploring other approaches. I'm not particularly fond of the useEffect hook approach here. It seems a bit to complicated and not very efficient.

Next, I think I'm going to try using the block attributes instead. I was avoiding it because attributes doesn't seem to be the place to store info that isn't used in the final render (like editing UI), but I found that the margin/padding block support already uses this method. They have an issue with accidentally persisting the values (#31839), but in this case the value does need to be persisted.

Another thought I had was to create a system that blocks can use to supply a function to be run to indicate if the block support should be active (type isSupportActive = (blockId: string, attributes: Object) => boolean). The block support can call that function with the block id and block attributes to compute the value per block to hide or show the support. However, right now I don't know where that function could be saved such that it's defined in block edit and available to call in block supports.

[jasmussen]

I started to on some of the newer PRs. My workflow was already set up for using my fork, so I just kept doing that. But I suppose it's a bit easier to check out and test a branch from here rather than my fork. :)

Do what works best for you :) — just stating you deserve it at this point.

[ajlende]

@jasmussen The cover block disables the buttons instead of hiding them like on the image block.

Disabled seems the way to go in my opinion, but do you think it would be too confusing to have only a disabled duotone toolbar button when everything else is hidden?

[jasmussen]

Disabled seems the way to go in my opinion, but do you think it would be too confusing to have only a disabled duotone toolbar button when everything else is hidden?

Good question. Thinking high level, not just about Duotone:

• I tend to think that disabled is correct to use if the feature is enabled but not functional for whatever reason. That way we can provide a tooltip explaining why the item is disabled.
• If the feature itself is disabled, for example through Global Styles, the button should be removed entirely. No need to confuse here.

However there's a bit of mixed usage in the editor at the moment, and some gray areas to consider. For example, "Replace" does not make sense to show and gray out on an image placeholder block, because the entire purpose of the setup state is to place.

In the case of the Cover block, I think it's actually the odd one out:

Most of those toolbar buttons aren't very useful, especially compared to those in the setup state itself. I lean towards this being a better Cover solution:

In other words, I think you've stumbled upon a topic that is worth discussing and addressing in a systematic way separately. But for now, I think it would make the most sense to just hide the duotone button on the setup state entirely, and show it first when an image has been added. What do you think?

[ajlende]

I think I've landed on the approach that I'm going to be happiest with.

Hiding the duotone button is done via block attributes and uses the same location that padding UI controls are set. Blocks that wish to start with duotone hidden can update the default attribute for style.ui.duotoneHidden.

Since the cover block and image block have many places which set the media url, having the useEffect hook makes it so that you don't have to remember to update the duotone UI control everywhere.

The control is duotoneHidden instead of just duotone because the value to hide the button had to be truthy since the padding UI controls strip all falsy values, not just undefined from the attributes.

[ajlende]

Admin e2e failures are broken snapshots now that <!-- wp:image {"style":{"ui":{"duotoneHidden":true}}} --> is saved. I'll get them fixed in the morning—don't let that hold you back from reviews.

[ajlende]

It seems that using useEffect breaks undo functionality which leaves us with no choice but to update the attribute everywhere in the same setAttributes that the media URL is set. @youknowriad do you know of a way to prevent this setAttributes from creating an undo level so we don't have to do that? Or am I just going to have to go back to using a custom value in a store?

EDIT: I opened up a new PR with the alternative approach that sidesteps the issue in #33466.