Typography: Make title blocks apply typographic styles consistently #65307
Conversation
aaronrobertshaw
added
[Type] Bug
|
I'm not sure whether this proposal would be considered a breaking change if themes relied on previous broken behaviour. I'm more than happy to hear any thoughts or concerns. Until this aspect of the PR reaches a consensus, I've left it in a draft state. |
Good point! Thanks for getting this PR up quickly. I'll likely run out of time to give it a proper test today, but will take it for a spin on Monday. |
|
Size Change: -529 B (-0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the follow-up here @aaronrobertshaw, conceptually I think this is the right way to go, and it means that we're saying that these blocks effectively are headings, rather than thinking of them as links. That greatly reduces the complexity of dealing with these blocks, IMO.
This is testing well for me:
✅ The blocks are no longer affected by overall link element styles
✅ Global heading element styles (e.g. letter spacing) apply to the Site Title and Post Title blocks where those blocks don't set those styles
✅ Block level styles in global styles override heading element styles as expected
✅ Block instance level styles continue to override block-level in global styles rules as expected
So, this is all looking good to me so far.
Until this aspect of the PR reaches a consensus, I've left it in a draft state.
Would it be worth doing a wider ping to the design or outreach groups for feedback, just to get a sense of whether anyone thinks it's a risk switching to always inheriting from the heading's styles?
aaronrobertshaw
force-pushed
the
fix/title-blocks-typography
branch
from
296a3c4
to
5472977
Compare
Definitely, at least now its had a basic sanity check by a few people 👍 I've given this PR a rebase so tests should pass soon. @WordPress/gutenberg-design and @WordPress/outreach it would be great to hear your thoughts on the approach in this PR 🙏 Effectively, as it stands in trunk, we can't reconcile link ( This PR explores treating Site Title and Post Title blocks as only "headings" i.e. even if they are set to contain a link they won't pick up global link element styles that might prevent other styles applying cleanly. See the PR description and linked issue for more context. |
|
Flaky tests detected in 5472977. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10877435424
|
|
Haven't had a chance to fully test yet, but why add this to CSS files instead of the Core/Gutenberg default For testing instructions, I'd add something like this to test from TT4's {
"styles": {
"blocks" {
"core/site-title": {
"color": {
"text": "blue"
},
"elements": {
"link": {
"color": {
"text": "red"
}
}
}
}
}
}
}Also test custom typography on both the heading wrapper and nested link. |
|
My biggest worry here is that it's going to break the colors for some themes. General link colors should always apply unless there is more specific styling for the Post and Site Title nested links. Breaking these expectations while working on themes is annoying. I'd avoid inheriting the color by default because:
I feel like the typography changes wouldn't have much of an impact, but I may be missing some scenarios. For me, though, I wanted to ensure that what I set in {
"$schema": "https://raw.githubusercontent.com/WordPress/gutenberg/trunk/schemas/json/theme.json",
"version": 3,
"styles": {
"blocks": {
"core/site-title": {
"color": {
"text": "purple"
},
"typography": {
"fontSize": "19px",
"fontStyle": "normal",
"fontWeight": "500",
"textTransform": "uppercase"
},
"elements": {
"link": {
"color": {
"text": "red"
},
"typography": {
"textDecoration": "none"
},
":focus": {
"color": {
"text": "blue"
},
"typography": {
"textDecoration": "underline"
}
},
":hover": {
"color": {
"text": "orange"
},
"typography": {
"textDecoration": "underline"
}
}
}
}
}
}
}
} |
|
Thanks for the questions and calling out the color styles @justintadlock 👍
The CSS is there to handle block instance styles coming from block supports. These need to be present for classic themes as well which won't have access to the default theme.json and global styles stylesheet. This PR consolidates those existing styles into a single rule.
The color inheritance for the Site Title block's link is already there in trunk. I did copy the Site Title's inheritance styles to the Post Title block and missed that it didn't have the color inheritance. That does raise a question though shouldn't these blocks be consistent? I can see that the use cases for the Post Title (within query loops etc) might be different than a Site Title so perhaps they shouldn't 🤔
I can remove the new
This was an oversight of mine once the styles settled.
That's correct. The style changes here only affect how global heading and link element styles apply by default. The global block type styles are enqueued after the library and directly target the block and inner elements as per the definition. FWIW, I believe you can still recreate the original typography issue by defining font-size and line-height on one of these blocks and then also defining the same typography styles on the inner link. In my view, that's a case of "doing it wrong" though we don't need to optimize for. I'll push a tweak removing the color inheritance for the Post Title block shortly. Let me know what you think of the end result 🙏 |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Yes, I was more thinking about post titles with color changes. The site title doesn't need to change. I don't think consistency is important here because post titles have so many more use cases. Update looks good to me. I'm not seeing any breakage across a few themes. |
There was a problem hiding this comment.
+1 regarding this working as expected and fixing the referenced issues.
I think this is a good move as expressed in #65169, and it looks like we're only hoping for a sanity check on the move to treating Site and Post Title blocks as headings. To that effect, it might be a good idea to ship this with 19.3 RC, due tomorrow, to get some actual usage and exposure. From there, we could contrast our concerns regarding breakage with some real-life usage.
|
Thank you everyone for the reviews and testing 🙇
Agreed. I'll get this merged. Happy to spin up any necessary follow-ups when the time comes. |
Fixes: #65169
Addresses issues introduced by #64911
What?
When title related blocks (Site Title and Post Title) are set to contain a link, typographic styles from the wrapping heading are enforced on the inner link.
Why?
See discussion here: #65169 (comment)
TL;DR
elementswithin a block type definition in theme.jsonheading > linkstructure and so global element styles for links and headings conflictHow?
achildren #64911 (fixes inconsistent styling issue)Testing Instructions
fontSizefrom TT4's Site Title styles in theme.json.Example block markup for testing
Screenshots or screencast
Demo adjusting global link, heading element and block type styles
Screen.Recording.2024-09-13.at.3.39.26.PM.mp4