-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typography block support: add typography support and defaults #34064
Typography block support: add typography support and defaults #34064
Conversation
Looks good -- but why no line height on list and preformatted? |
Thanks for testing!
At the time, there wasn't a clear answer as to whether we should include line height for blocks that do not yet explicitly support in the typography settings. I thought it safer to only add defaults for the current settings until there was a requirement or direction, or at least until we can do a spot of design testing. A more recent comment from @jasmussen in the Typography: Switch to ToolsPanel for display UI PR seems to support that view. Happy to give line height support a quick test for these blocks as well if we'd like to enable it immediately. I don't expect too much trouble from such a property. There's also the option to go with currently-defined properties as defaults and follow up after testing/consultation. |
b591fcf
to
1fef971
Compare
Rebasing this PR, I noticed that the post date block has introduced For that block in particular, I don't think it looks too busy with the fontWeight enabled as well. It could be useful since there are no inline controls to adjust font weight for that block. |
Size Change: +869 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
I would have expected list item to support line height, but based on Joen's comment we could leave it out and come back to it. |
2e81026
to
36ca314
Compare
36ca314
to
1e56e35
Compare
"__experimentalFontWeight": true, | ||
"__experimentalDefaultControls": { | ||
"fontSize": true, | ||
"lineHeight": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add "weight" as a default and remove line-height
We removed font-family in a previous commit, but it broke several blocks validations: #35604 (comment) My proposal was to leave font-family for the blocks where font-family already exists and then, if we decide to remove it for these blocks, follow up with individual PRs for each deprecation (around eight). |
Hello 👋 Is the call with Matias a call for Gutenberg contributors? Thanks |
Ciao @francescamarano! I think it's a quick, on-the-fly sort of thing, so not a formal contributor meeting. Anything we learn will be posted right here. Thanks! |
@francescamarano It's an informal meet and greet for our team. Not specific to Gutenberg or this PR. Anything related to it that comes up we'll post here in detail. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @ramonjd, thanks for working through all these! They're testing well for me. I've noted that a couple of the blocks behave in slightly awkward ways (e.g. it'd be great to have more granular control over typography within elements of the block), but I don't think any of these are blockers for getting this PR merged.
The one issue that we might want to tweak, is that the post comments count block currently opts-in to text transform, but since it always renders a number, I think that might not be necessary?
My full testing notes are below. Apologies for the big list!
✅ Button block: Font size shown by default
✅ Code block: Font size shown by default, all others working well
✅ Heading block: font size, appearance, text transform shown by default, all others working well
✅ List block: font size shown by default, all others working well
✅ Navigation block: font size shown by default, all others working well
✅ Paragraph block: font size shown by default, all others working well
✅ Post author block: font size shown by default, all others working well
✅ Post comment date block: font size shown by default, all others working well
✅ Post comments count block: font size shown by default, all others working well except text transform which might not make sense if it's always just a number? ❓
❓ Post comments form block: mostly works but the nested h3
element doesn't get targeted by some of the Typography settings set at the container — left a note, but likely things to be followed up with separately for that block
✅ Post comments link: font size shown by default, all others working well
❓ Post comments block: mostly works but I ran into a similar issue as with the Post comments form block: it's a more complex block, so applying the Typography controls at the container level applies to all elements within it. But, since it's server-rendered, it's maybe not such a big issue to have it enabled
✅ Post date block: font size shown by default, all others working well
✅ Post excerpt block: font size shown by default, all others working well
✅ Post navigation block: font size shown by default, all others working well
✅ Post terms block: font size shown by default, all others working well
✅ Post title block: font size, appearance, letter case shown by default, all others working well
✅ Pre-formatted block: font size shown by default, all others working well
✅ Pullquote block: font size and appearance shown by default, all others working well
✅ Query pagination next block: font size shown by default, all others working well
✅ Query pagination prev block: font size shown by default, all others working well
✅ Query title block: font size, font appearance, text transform shown by default, all others working well
✅ Quote block: font size and appearance shown by default, all others working well
✅ Site tagline block: font size shown by default, all others working well
✅ Site title block: font size, line height, appearance, letter spacing, text transform shown by default, font family also works well
❓ Table block: all the opted-in settings work, but because we don't have separate typography controls for table heading versus content and footer, is it an issue that we're setting all these at the container level? I could imagine folks finding it frustrating that they can't control these things separately... but I suppose it's better to expose some control than none!
✅ Term description block: Font size shown by default, line height can be accessed
✅ Verse block: Font size and appearance shown by default, all others working well
As I mentioned, I think some of these questions would be good things to explore in follow-ups over time, but not blockers for getting this larger PR merged in the shorter-term 👍
"lineHeight": true | ||
"lineHeight": true, | ||
"__experimentalFontStyle": true, | ||
"__experimentalFontWeight": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found font weight and font size, etc, didn't quite work as I expected with this block, because it's a more complex block. E.g. the font weight gets applied at the container, but there is a nested h3
that doesn't wind up being affected by this:
Because this is a server-rendered block, I wouldn't consider these details a blocker — I think personally it's fine to opt-in to these Typography supports and defer fine-tuning the behaviour to follow-ups further down the track.
since the rendered output is a number
Thanks again for the always helpful review, and the eagle eyes.
This is what I found, and also came to the same conclusion as you: the typography controls are nice to have, but it seems like another one of those blocks that demand finer control over the inner elements. 🤷
I'll work on immediate follow ups for these. 🍺 |
@francescamarano The main points that came up in a chat with Matías about this:
|
Thanks! Hopefully, the Webfonts API will make it to WP 6.0 🚀 |
Resolves: #35604
What this PR does
This PR is a follow-up to #33744, which updates typography controls to use the very excellent ToolsPanel.
We're trying to achieve the following:
Testing
Get comfortable.
Rebase this branch on top of
origin/update/typography-support-to-use-new--panel
Create a new post and insert the follow blocks:
For each block, please check that:
Screenshots
Lo! The Verse block opts in to
fontSize
only!The paragraph opts in to both
fontSize
andlineHeight
. WOOT!Checklist:
*.native.js
files for terms that need renaming or removal).