Add elements support to the typography panel in global styles #36718
Conversation
youknowriad
added
the
Global Styles
youknowriad
requested review from
oandregal,
jorgefilipecosta,
jasmussen,
mtias and
critterverse
|
Size Change: +1.33 kB (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
Nice one. Before: After looks excellent, except for the font size being inherited in the tiny little preview:
That'd be my only niggle. CC: @ciampo for the |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
Thanks for the ping! The |
There was a problem hiding this comment.
Thanks for the PR Riad!
I'd echo Joen about the preview text. After reading the issue I'd expect a preview card like this from this comment:
Otherwise this looks good.
| title: __( 'Text' ), | ||
| }, | ||
| link: { | ||
| description: __( 'Manage the fonts and typography used on the links.' ), |
There was a problem hiding this comment.
I guess these descriptions should be more consistent and change only the part on the site|links, no?
There was a problem hiding this comment.
I'm happy to use any proposed descriptions here :)
There was a problem hiding this comment.
So I guess we should remove the export here?
|
@ntsekouras this PR didn't update the design of the typography panel itself, just the elements part. |
To clarify the only part I was referring to, was this: This is not a part of this PR? |
Yeah, h1-h6 are different elements, so if we want to expose them all in the UI for the user to change we need to do it individually. |
No, it's not but I can add it, it shouldn't be that hard. |
| @@ -54,8 +54,10 @@ function useHasLetterSpacingControl( name ) { | |||
| ); | |||
| } | |||
|
|
|||
| export default function TypographyPanel( { name } ) { | |||
| export default function TypographyPanel( { name, element } ) { | |||
There was a problem hiding this comment.
Hey, I'd like us to revisit this conversation #35264 (comment)
What are your thoughts about passing the underlying functions useSetting, useStyle, and getSupportedGlobalStylesPanels the parameters path and context? The context would contain any extra metadata we need (source, block name, element, etc). I've recently introduced this change for the server-side API to be future-proof in regards to addressing different templates down the road #36610
Not a blocker for this PR, although I'm now more convinced we should do it for 5.9 than I was a few months ago when we first discussed it.
There was a problem hiding this comment.
yep, I'm fine with that change but this is an internal API for the moment, so there's no real urgency yet.
|
Nice! Super small note that the original designs indicate underlined text in the inline link preview. Is that something we could add here?
|
I feel like this should be a theme's decision, do we have a block support already to underline things? Maybe we should show it in the typography panel to allow users to enable/disable underline for texts. |
There should be a text-decoration tool but it was not enabled as a cosmetic one for non-link items. |
|
Links are underlined by browser default, shouldn't they be underlined until the theme or user removes them, |
|
I've added the preview card and also forced links to be "underlined" for now until we add a text decoration control there. |
|
What's missing here, can we get this iteration in? |
|
To me this looks good to land as an iteration, except for one thing. We still have the font size inherited in this tiny little thumbnail:
I think we need to just enforce a font size, say 13 or 14px there. We can still inherit the font and style. |
|
Works for me, I'll update it. |
|
Yes, the size is meant to be hard-coded in the small preview. |
|
Updated and used 14px there. |
|
Can we bring color as well if it exists for that element's preview? |
|
@mtias sure 👍 |
|
Colors added (both background and text) |
|
Thanks! :) |
| }, | ||
| link: { | ||
| description: __( 'Manage the fonts and typography used on the links.' ), | ||
| title: __( 'Link' ), |
|
Will 5.9 also include a way to change the Headings as shown by @critterverse? |
|
@briceduclos not at the moment. FSE don't allow to change all headings at the same time, we can potentially have an entry per heading level, I'm waiting for more design input there. |
|
We need to first establish what "headings" mean since it can span multiple blocks (heading, post title, archive title, comment title, etc). For that we need to first focus on the block API aspects of designating things as headings (which also includes table of contents, outline, etc). Once we have a plan there we can incorporate it as an element. Link and Text are already established and working. |
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
|
Agree that changing headings from body copy is pretty important. Any updates on this feature being included? |
|
I see a + sign here to add more elements for custom styling, though i'm missing this in the editor atm. Will this be added? I think it would be a nice thing to have |
closes #36546
This PR adds an element selector to the typography panel to match the proposed global styles design.
The panel is only enabled in the root of the global styles for the moment. (We don't have a clear way to say which blocks supports which element)
Notes
If we want to add headings there, I think we'd need a row per heading level if I'm not wrong. right @jorgefilipecosta @oandregal ?