Inspector Controls: Rephrase, polish, and make consistent color labels. #30075
Conversation
jasmussen
added
the
[Feature] Inspector Controls
jasmussen
requested review from
ajitbohra,
ellatrix,
mkaz,
nerrad,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: -6 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice!
Looks good although I have just one minor question regarding the change of Icon background color to Background color on the social links block.
That color control is not actually setting the social links block's background color, only the background for the icons within it. Would it be better to leave it as Icon background color as it is more accurate?
Happy to defer to you if greater consistency outweighs accurately labelling what the control affects 🙂
|
That does actually make sense. Thanks. |
|
I'm thinking about how to generalize those labels so we could have a more structured approach when injecting UI controls with
There are exceptions like the Button block and Social Links block where we have quite a unique approach:
I think it's a bit confusing in the Button block and it shouldn't use the name background since it's different from the background you know from the Group or Heading blocks. Should we use a consistent naming that would work for both those blocks and better express the intent like |
|
Could we use "Item background"? |
Yes, |
|
👋 If the same label names in the color hook can be the same in use for all the blocks (including social links) we won't need to implement #29155 (it was created because in #28913 we wanted to migrate some blocks -including social links- to use the block support mechanism instead of the custom controls and labels). |
|
@nosolosw, it might be as simple as extending what you can see for links in the Paragraph block: gutenberg/packages/block-library/src/paragraph/block.json Lines 33 to 35 in 85bf3a7 For the Social Links block you could have: "supports": {
"color": {
"foregroundItem": true,
"backgroundItem": true
}
}and we would only need to extend the handling in the color hooks in the block editor package. |
|
Feel free to push any changes to this PR if that's helpful. |
|
We can also separate things out, as this PR touches a slew of other things. |
|
Let's agree on the direction first, I don't know if what I propose makes sense with everything else that already exists 😄 |
|
@gziolo To make sure I follow, there're two things here:
Is that correct? If we were to rename them, I'd think we'd want Does this help? |
|
@nosolosw, I don't think this is what I proposed:
I think that the existing default values are here to stay as they cover 90% of cases:
We could add more options, but it might be as well confusing for people using API.
Yes, as discussed in other places, we shouldn't cover labels in the |
|
I've closed #29155 (comment) in favor of this one. Talked with Greg about this: it looks like the first step is just aligning the labels in use by the UI controls generated by the hooks and the custom labels. We won't offer the ability to have custom labels per block in the UI controls generated by the hooks at the moment. |
|
@nosolosw Alright, I have to work on some other things at the moment, but feel free to entirely take over this branch if it's useful to you. |
|
What are next steps here? |
gziolo
changed the title
| @@ -68,9 +68,7 @@ function ColorPanel( { settings, clientId, enableContrastChecking = true } ) { | |||
| const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState(); | |||
| const [ detectedColor, setDetectedColor ] = useState(); | |||
|
|
|||
| const title = isWebPlatform | |||
| ? __( 'Color settings' ) | |||
| : __( 'Color Settings' ); | |||
There was a problem hiding this comment.
@hypest, is there any reason this differs in the mobile app. Are there tests that depend on it that need to get updated?
There was a problem hiding this comment.
As far as I checked, we don't have any test that relies on this. I checked the commit that introduced this change and it doesn't look like the decision of using different values has to do beyond a naming convention. Being this said, I'd say it's safe to continue with this change 👍 .
@hypest do you agree?
There was a problem hiding this comment.
Yeap, I agree. To be on the safe side I created this gutenberg-mobile PR to run all tests from the native mobile side of things. Edit: native mobile tests are green 👍.
There was a problem hiding this comment.
👋 @iamthomasbishop , what's your take on the use of Sentence case here? Will it be a problem for the mobile apps? See for example this diff.
There was a problem hiding this comment.
Another way to think about it is: is title-case worth extra code across the entire project?
There was a problem hiding this comment.
In Polish you don't even have the choice, it's a sentence case on all websites, sometimes they use uppercase for highlights 😄
There was a problem hiding this comment.
what's your take on the use of Sentence case here
@hypest We're slowly moving towards sentence-casing on mobile, so we can remove the mobile-specific code here.
There was a problem hiding this comment.
✅ all systems go then! I will go ahead and merge this PR, thank you all!
|
Thank you for the approval! Did the thoughts here have any impact on next steps? #30075 (comment) |
I think we are fine. I prefer we land with a more general approach rather than trying to account for more distinct use cases. We need to double-check with the mobile team while they picked a different label in the mobile app before proceeding. See #30075 (comment). |
|
Cool. Feel free to press the green button when you feel it's time then, thanks! |
Description
Color panel labels are a bit of a mix across panels and even across web and mobile. This PR attempts to clean it up.
Before:
After:
The motivation here is that the "settings" part of "Color settings" is reduntant information, and that we use sentence case for labels everywhere else.
Types of changes
Checklist: