Fix: Block panel should be hidden taking into account gradient block support #33311
Conversation
This file contains 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
jorgefilipecosta
added
[Type] Bug
|
Size Change: +35 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
|
Hey, this fixes the steps I've described at #33311 but I've found that still doesn't respect a block's color model. Let's say a block only supports gradients and link: "supports": {
"color": {
"text": false,
"background": false,
"gradients": true,
"link": true
},
}When the theme declares the following preferences via {
"version": 1,
"settings": {
"color": {
"custom": true,
"palette": [],
"customGradient": false,
"gradients": [],
"link": false
}
}
}I'd expect the color panel to be hidden, as the link is set to false and the gradients are disabled (both presets and custom). Instead, it's present but empty. The same happens when |
oandregal
added
the
[Feature] Design Tools
oandregal
reviewed
Jul 9, 2021
| @@ -12,12 +12,19 @@ import ContrastChecker from '../components/contrast-checker'; | |||
| import InspectorControls from '../components/inspector-controls'; | |||
| import { __unstableUseBlockRef as useBlockRef } from '../components/block-list/use-block-props/use-block-refs'; | |||
|
|
|||
| const EMPTY_OBJECT = {}; | |||
| const DISABLE_GRADIENT_SETTINGS = { | |||
There was a problem hiding this comment.
I'm a bit confused about the implementation of the color components, so I may be missing the whole context. I do wonder why we need to keep this data here? Don't we have access to useSetting(color.customGradient) and so on? I see that in use at panel-color-gradient-settings.js.
There was a problem hiding this comment.
We want to force the disable of gradients so we explicitly pass the settings that disable them. useSetting(color.customGradient) on paragraph may still return true if gradients are enabled but the gradients are not supported on the paragraph.
There was a problem hiding this comment.
I see. So what we're doing is passing those settings as empty to the underlying components when the block doesn't support gradients. Why don't we need to do the same for solids?
I also wonder if we could unwrap a bit all these layers/components and do the block support check earlier.
There was a problem hiding this comment.
Why don't we need to do the same for solids?
Because we don't have a way of supporting gradients without supporting solid colors. Even solid colors are not supported the support mechanism is not triggered and the code never reaches here,.
I also wonder if we could unwrap a bit all these layers/components and do the block support check earlier.
We do support checks early, for example, if colors are not supported our code never reaches here but to know if the component should not render at all we would need to inspect the palette of the specific block and other settings e.g: custom etc, etc and that is already done inside the PanelColorGradientSettings, we would be repeating all the checks this component does early which is a lot of code repetition. This solution seems to be the simplest one and the one that does not involve code repetition.
|
Hi @nosolosw, When the theme declares: The theme is not providing any color palette and is saying the palette should be empty, but the theme is also saying the user should be able to pick any color using the custom color picker. So the UI is correctly displaying without any predefined color and allowing the user to pick a custom color.
In this case, the theme is saying the user should not be able to pick a random color so the custom color link should not be available but the user should be able to pick a color from the palette. So the component renders with the palette and without the custom color link. Both things seem to work exactly as expected. The custom flag is just a flag that enables the user to pick or not a random color it is not a flag to disable color functionality. |
|
@jorgefilipecosta Note that I mentioned #33311 (comment) that the block only supports link and gradients, and the theme has disabled both of them. I should have attached a screenshot (I modified paragraph block.json for testing):
|
|
I've prepared #33369 that takes a different approach and fixes the two issues: #33304 and #33311 (comment) |
|
The alternative #33369 has been merged so this can be closed. |
oandregal
deleted the
fix/color-panel-should-be-hidden-taking-into-account-gradient-block-support
branch
oandregal
removed
the
Backport to WP 6.7 Beta/RC
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.
We had a bug on the color supports hook: If a block did not support gradients and colors are disable it is not possible to do anything on the color panel and it should not render. But we did not checked for gradient support on the supports hook so unless both colors and gradients are explicitly disabled the panel would appear.
Fixes: #33304
How has this been tested?
I repeated the steps on #33304 and verified the issue does not happen anymore.