Global Styles: Caption element UI controls #44094
Conversation
MaggieCabrera
changed the title
MaggieCabrera
added
the
Global Styles
|
Size Change: +226 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
MaggieCabrera
force-pushed
the
add-caption-gs-ui
branch
from
cc6fcc4 to
14a2f8b
Compare
MaggieCabrera
requested review from
carolinan,
scruffian,
noisysocks and
mirka
There was a problem hiding this comment.
Thanks for working on this. Everything worked as expected in my testing.
I noticed that to add colors require a lot of new (probably duplicated) code, whereas adding typography just works. Is it possible to abstract the way that colors works in the same way that typography has been, so that we don't need to duplicate all this code every time we add a new element?
|
One thing we need to support for global captions is text alignment. |
Looks like some settings are missing from this panel, but that might be covered by #44180 |
MaggieCabrera
force-pushed
the
add-caption-gs-ui
branch
from
14a2f8b to
d25cace
Compare
I'm working on this and I noticed that the links element implemented pseudoelements but the same was not done for the buttons, abstracting the component will help with that and solve it for future elements. It will make this PR a bit wider in scope but I hope it will still be easy to test when I'm done. I can split it if needed. |
|
Looks like the headings and links need a bit more work to use the new component I included here, so we better cover them in a follow up. This PR should be good to test. We must ensure that button, text, and caption all work as intended in GS, both with theme.json values and user changes. This is the theme.json I used to test while using emptytheme: |
MaggieCabrera
requested review from
getdave,
draganescu,
talldan and
andrewserong
scruffian
force-pushed
the
add-caption-gs-ui
branch
from
1c60d8e to
8094107
Compare
There was a problem hiding this comment.
This is very cool @MaggieCabrera! It's testing well for me, too. I just left some nit-picky comments in the ScreenColorElement component. They're very much just some thoughts I had about how the code is organised, so not a blocker for landing this PR, and could potentially also be code quality changes to be explored separately if need be.
In terms of using the feature, I think it's a great idea exposing the controls for the Caption element. One concern I have is about being able to adjust the background color. In a theme that doesn't provide pleasing padding for the figcaption element (like TT2), when I apply a background color, I immediately wanted to then also be able to adjust the padding of the caption, as without that control, I found it hard to make the caption look pleasing:
I suppose part of the challenge with background colors for the Image block is that there is a space between the caption and the image, so depending on the styling someone is going for, they might either want to be able to remove that space between the caption and the image, or they might want the background color to be applied to the parent figure element instead so that it naturally captures the background of both the image and the caption 🤔
Other than those fiddly issues surrounding the background color, the text color and typography controls were all testing nicely for me! ✨
| supports.includes( 'color' ) && | ||
| isTextEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| textColorElementSelector = 'color.text'; |
There was a problem hiding this comment.
Is this the only place we mutate textColorElementSelector? If so, would it work to do the calculation when we define that variable, and switch to a const?
Alternately, what if we moved the selector to be a key of the element in the elements object above (so, elements.text.textColorElementSelector?
| isTextEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| textColorElementSelector = 'color.text'; | ||
| isBackgroundEnabled = false; |
There was a problem hiding this comment.
Similarly if we are hard-coding particular elements to disable certain features, would it work to add it to the elements object?
| switch ( element ) { | ||
| case 'button': | ||
| hasElementColor = | ||
| supports.includes( 'buttonColor' ) && | ||
| isBackgroundEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| break; | ||
| case 'text': | ||
| hasElementColor = | ||
| supports.includes( 'color' ) && | ||
| isTextEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| textColorElementSelector = 'color.text'; | ||
| isBackgroundEnabled = false; | ||
| break; | ||
|
|
||
| default: | ||
| hasElementColor = | ||
| supports.includes( 'color' ) && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is potentially a little nit-picky, and it doesn't have any impact on the overall functioning of this code, so feel free to ignore this comment!
Some of the global styles files get complex pretty quickly as we add additional elements and controls, and looking at the changes here made me think of a couple of small changes that might improve readability. Is there an opportunity here to add a useHasElementColor hook to this file, similar to how the border-panel.js and dimensions-panel.js files have a bunch of useHas hooks?
We could then potentially swap out the switch statement to use that hook, where the internals might be a shorter series of if statements:
if ( solids.length > 0 || areCustomSolidsEnabled ) {
if ( element === 'button' ) {
hasElementColor =
supports.includes( 'buttonColor' ) && isBackgroundEnabled;
} else if ( element === 'text' ) {
hasElementColor = supports.includes( 'color' ) && isTextEnabled;
} else {
hasElementColor = supports.includes( 'color' );
}
}Ideally ScreenColorElement would then either get its settings via calling other hooks, or do a look-up to the elements object.
My feedback here is very much influenced by my personal preference that I find switch statements challenging to read, so definitely not a blocker to this PR landing.
There was a problem hiding this comment.
Breaking this into a separate hook sounds good to me 👍
Along with making it easier to grok. It might open the door for writing tests as well, should we ever desire them.
My feedback here is very much influenced by my personal preference that I find switch statements challenging to read
For me, I think it might be the nested and repeated check for ( solids.length > 0 || areCustomSolidsEnabled ) that lowers the readability more so than the switch. That's just another personal opinion or preference though 🙂
|
I've added a couple more folks as reviewers (Glen and Aaron), but just as an FYI / for visiblity since there's been previous conversations about background colors, spacing, and the Image block 🙂 |
|
Thanks for the heads up on this one @andrewserong, and all the work to date @MaggieCabrera 👍 I'll endeavour to give it a look through Monday. For now, though, the first thing that came to mind were some discussions around captions becoming their own standalone block. There was also a PR opened for this a while back #31823. There could still be an argument for both this PR's caption elements and the standalone block, e.g. for the user to be able to style individual captions vs also being able to blanket target captions in 3rd party blocks. Captions were also raised in the issue tracking typography block support adoption. Depending on the consensus here that might need updating to state we won't be adopting typography supports for the caption-related blocks. |
There was a problem hiding this comment.
Thanks for the great work on this @MaggieCabrera 👍
In general, this is testing pretty nicely for me.
Like @andrewserong I was caught out a bit by the gap between the image and its caption when setting a background color. FWIW, in the editor, the resizable box handle appears to slightly exacerbate the size of the gap compared to other embed blocks. That's not an issue for this PR but something we might need to iron out in follow-ups.
In terms of padding within the caption, line height only helps us with vertical spacing, we still have problems with left or right-aligned captions.
For example:
The need for padding support is probably another argument in favour of refactoring our blocks' captions into their own standalone blocks. Then they can have spacing supports as well as typography and color.
I'd still see a role for the caption within the Elements API, but that might be reduced to:
- Only temporarily being the primary means of configuring caption styles
- Once we have standalone caption blocks, we'd recommend the caption element only for basic styling of captions added via 3rd party blocks etc that don't themselves use the new caption block.
Having two approaches with considerable overlap might introduce its own problems, but I suspect they'd be manageable with the right selector for the standalone caption block. We also have a precedent with the heading element and block.
The only other issues I encountered were around a theme author wishing to disable color controls.
When disabling text color controls via theme.json, it still shows regardless within the captions element panel but not the other elements. Even if it did omit the text control as it does with background when it is disabled, we'd end up with an empty panel and no back navigation.
The empty panel issue isn't unique to this PR, though, it happens for the other color panels on trunk. We might need checks to skip rendering the initial colors option or individual elements.
This might be suited to a separate follow-up to allow for a more focused discussion on the desired approach and unblock this PR.
Happy to hear everyone else's thoughts on this one.
Here's what I see when switching from enabled color UI to disabled.
| Enabled | Disabled |
|---|---|
Screen.Recording.2022-10-24.at.8.07.42.pm.mp4 |
Screen.Recording.2022-10-24.at.8.08.05.pm.mp4 |
| switch ( element ) { | ||
| case 'button': | ||
| hasElementColor = | ||
| supports.includes( 'buttonColor' ) && | ||
| isBackgroundEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| break; | ||
| case 'text': | ||
| hasElementColor = | ||
| supports.includes( 'color' ) && | ||
| isTextEnabled && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| textColorElementSelector = 'color.text'; | ||
| isBackgroundEnabled = false; | ||
| break; | ||
|
|
||
| default: | ||
| hasElementColor = | ||
| supports.includes( 'color' ) && | ||
| ( solids.length > 0 || areCustomSolidsEnabled ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Breaking this into a separate hook sounds good to me 👍
Along with making it easier to grok. It might open the door for writing tests as well, should we ever desire them.
My feedback here is very much influenced by my personal preference that I find switch statements challenging to read
For me, I think it might be the nested and repeated check for ( solids.length > 0 || areCustomSolidsEnabled ) that lowers the readability more so than the switch. That's just another personal opinion or preference though 🙂
| const [ color ] = useStyle( 'elements.caption.color.text', name ); | ||
| const [ bgColor ] = useStyle( 'elements.caption.color.background', name ); | ||
|
|
||
| if ( ! hasSupport ) { |
There was a problem hiding this comment.
Should we also skip the panel if a theme author has chosen to disable text and background color controls in Global Styles?
I appreciate this follows the other elements that miss this as well. So it would likely be better as a follow-up.
|
There's something I don't understand here:
Can't we do exactly that but as an element? The button element supports spacing settings in theme.json, and probably the caption does as well, we just need to surface that to the UI, I don't see the need to make them blocks for that. |
That's correct, it could be styled via theme.json directly, and as you note, there isn't currently a UI for padding for elements in the Global Styles sidebar unless I missed it. What happens when a user wishes to override a style for a particular caption? They can't edit the theme.json, and the elements UI in Global Styles would only work across the board or for all instances of a given block type. If captions were a block, we'd get all the UI in Global Styles for free and fill the gap for end users. |
Yeah, it's on our radar to add the option
I see what you mean, I hadn't thought of that. If you think that's a use case that merits adding the new block then I think having the element as well is going to be a bit redundant and probably confusing if the theme.json alters one but not the other. |
|
This was a question we considered previously for the button block/element, and this comment by @mtias is helpful. I think if we apply those same criteria to the case of captions it doesn't make sense to have them as inner blocks. |
There would definitely be overlap, but it wouldn't be completely redundant. This was part of what I was referring to earlier. even with an inner block option for captions, you could still have a more complex block or 3rd party blocks that include a caption that isn't leveraging the new block. That could still be targeted via the caption element. As noted earlier, there's a precedent with this and the heading block/element.
Thanks @scruffian, appreciate the extra background.
There have been a few things coming up around captions and a potential block that might make this a slightly different situation.
So for me, some of the benefits of implementing a caption block would be:
I appreciate not everyone might value all those points the same. I do believe they are all worth considering on their own merit, separate to the decision on the button element vs inner block for the Search block. If we can address all of the above without a block, I'd be happy with that. It's not clear to me at the moment though how that would be possible. Thanks for the discussion towards trying to make sure we cover all bases for captions 🙇 |
|
Great discussion here!
If we go the route of treating captions in image and gallery blocks as elements, what's the path forward for adding padding, typography, color controls at the individual element level within the block editor? One of the things with the Button block is that it can be styled both via the elements approach in It'd be great to be able to click on a All that said, given the relationship between elements and the Button block works pretty well, I don't think of it as a blocker for this PR which is about controlling the display of captions globally. It'd be great to see if we can get padding support added though, so that the spacing with a background color applied can be controlled. |
Where are these requests out of curiosity? |
It did come up recently in an internal discussion, although my memory is failing me on the specifics of that conversation now, sorry. Even checked with the team, but only vague recollections were voiced there to. The ability to move the caption was floated in the PR trialling a caption block, though. Within Gutenberg there are a couple of issues related to moving the caption for the Table block: My recollection might also have been influenced from seeing the range of WordPress support forum posts asking things like how to move a caption "over", "above", or "on top" of images but meaning to overlay them on the image. Maybe a Block Style might allow for this, or even an on hover caption display, to be implemented once but leveraged across all images, galleries and embeds. While going on a quick dig through Gutenberg issues for ones related to "captions" the results get swamped pretty quickly by requests for better control over caption alignments. Would an inner block help offer better alignment control for captions as well? Some issues related to caption alignment include:
Hopefully, that all helps some in evaluating whether a caption block is worth it. |
|
Indeed, I don't think there's a compelling argument for reordering. Overlaid captions can be a style, but it actually makes the point that it should not be treated as a block given an overlaid caption doesn't make sense for tables. We should probably allow switching between overlaid or not for the image block alone, and use it by default on galleries. All in all the caption is not an independent unit nor something that needs ordering flexibility to warrant being a block with all the impact it carries (selection, display in list view, breadcrumb, etc).
Not really, since we are just dealing with text alignment which can be handled just fine by it being an element. |
|
Sounds like an element is the preferred option 👍 Should we extend the RichText component for captions so individual blocks can tweak all that gets set at the Global Styles level? Or restricting at all what's configurable for captions via global styles? With the current approach, users wouldn't be able to override font family, font size, line height, and some font weights. Do we also still see value in providing spacing control for the captions at both global styles and individual block levels? |
As long as you can style captions within the Site Editor—the same typographic supports we use for Headings/Paragraph— we're good imo.
I don't think this is necessary. Typography settings in Global Styles do not allow for Layout > Dimensions currently. I know captions are a bit different as there is not a block to manipulate the dimensions further, but I don't think this should be a blocker. I'll take this for another spin if we get the merge conflicts cleared up, and the PR updated with trunk. 🚀 |
Sorry, it's been a while! A lot has changed since I worked on this and I think the rebase is a bit of an ordeal, so I'm gonna try and re make this PR again to make sure we don't lose any changes made since we opened this PR, will update here with my work |
|
I'm sorry to move the conversation over to #49141 but I think it was best, since a lot has changed around GS since this PR opened. The new PR is more or less at the same spot this one was with a few minor bits of feedback applied to it from the discussions on this one. Please let me know what else I can do to improve it, and we can get it in and follow up with maybe adding padding controls to the element too on a separate PR. |
|
Solved in #49141 |
What?
Add caption elements to the list of elements that can be customized in the Global Styles UI.
Closes #44071
Why?
So far, this could only be achieved via theme.json, now we can control these elements with Global Styles too.
How?
Adding captions to the list of elements in the color and typography sections of Global Styles
Testing Instructions
I'm using
emptythemewith the following in theme.json:Go to global styles and change typography and color settings for the captions. Check that the frontend matches what you see on the editor.
Screenshots or screencast
Screen.Capture.on.2022-09-15.at.16-27-26.mp4