-
Notifications
You must be signed in to change notification settings - Fork 105
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
Improved type system #422
Improved type system #422
Conversation
Preview changesYou can preview these changes by following the link below: I will update this comment with the latest preview links as you push more changes to this PR. |
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. |
I am not comfortable with these changes as these are changes that @beafialho has said no to repeatedly. |
Does this PR closely match what I originally suggested after listing the font sizes I found in Figma? Yes. But there is a lot of team work, discussion, altering, testing and compromising from all parts behind the final decision that we have arrived at together. |
Question: is this something that can be addressed post beta-1? Or in a point release, 6.7.1? I see the benefit in reducing the reliance on custom font size presets especially when the visual diff is small. At the same time, there can be value in showing how the font size control can be used to add some diversity across patterns. |
typography-preset-x1.json cleanup -This I feel that I can approve and is even necessary. morning -I am unsure about removing the size for the tagline, since the default size in theme.json is smaller. But since the tag line is not in the design in Figma, I will refer to @juanfra who added the size. patterns/contact-centered-social-link.php, Old size above, new size below: patterns/cta-grid-products-link.php, patterns/heading-and-paragraph-with-image.php patterns/hero-book.php Skipping this because since the pattern was completely replaced I can not narrow down what the actual change is right now without spending too much time. patterns/services-three-columns.php The change to the spacing on the image needs a motivation. Theme.json A compromise would be to have them all in the font size control and not limit the control to the t-shirt size buttons. |
I believe that once we merge this one (with whatever naming we decide), we need to:
So that we address the concerns of this one. If we go that road, I believe this PR won't be necessary. The old XXL and XL will live on these new text styles, with whatever names we choose (the ones discussed here). So, people will still have control of any of these sizes via text styles. Having the
I chose |
This PR establishes some level of consistency across type. It's all over the place currently, with multiple layers of complexity. As I've mentioned above, we need to get a consistent systematic approach in place - then we can layer the nuances on top of that. Otherwise we're just building on an unstable foundation, which we've seen the fruit of from multiple attempts to "fix" the theme's typography. |
From my best understanding, this PR now makes use of the heading sizes and new text styles, to make progress on #286. @juanfra is that a correct assessment? Separately, any details aside, if these updates meet the spirit of the original designs, there is some simplicity to be had due to the use of those presets, rather than custom font sizes: With that, I'm not necessarily suggesting there's a tradeoff to be made. Just noting, that for the patterns that maintain their visual fidelity even after these changes, they will be easier to edit for it. To that end, I did a quick review, I pulled down this PR and compared patterns with those designed in Figma. Essentially, things look pretty good, but it's easier to compare trunk with this branch. The thing is, with the recent merge, some patterns also actually broke a bit, and this branch fixes it up. Consider banner with description and grid. Trunk: Branch: Figma: The only gotcha there, is the typographic widow of "flowers" standing on its own line. The easiest fix is to rephrase it slightly, to remove a word or two. It can just be:
Removing "beautiful". Most of the patterns look virtually identical. I did get a validation issue with "Content location and link": The "Hero book" looks a bit better in trunk and Figma than here. Trunk: Branch: Keeping the right padding on the right column is probably worth it, unless that breaks something, responsively. Hero Full Width Image looks like this in trunk: The first paragraph is a bit bigger in this branch: IMO both look good, but Figma has it smaller: Overlapping images and paragraph on right has its pill annotation fixed in this branch 👍 👍 For the rest, I couldn't tell a difference. Summary Possible edits to consider:
Sidebar: and I saw an issue that already had this tracked: we should audit both the filenames and the titles across every bundled pattern. Right now we have a mix of titlecase and sentence case, and prefixes with colons or not. |
100%. Pattern titles, categories, descriptions and keywords need auditing. Patterns should be more opinionated category wise, otherwise you end up shuffling among multiple categories that don't really line up with the user intent. I think it's fine if most are only in one category. There will be some concessions font-size wise; there's not a way around it—other than adding every font size from the Figma—but I do think this in more in line with the spirit of the original design, while also keeping complexity to a minimum. Here are the updated versions of those ☝️: |
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'd appreciate more people chiming in, but since it's nearing kid's bedtime, here's one green check from me.
Right, I was also proposing to have some level of consistency but with a different approach. I definitely agree, and I'm 100% sold on consistent systematic approach. I must admit I re-read my previous message, and maybe it's unclear. I recorded a screencast with audio, hoping that more clearer. I believe what I'm proposing has 1:1 visual fidelity with what we have in Figma, and we can use it to address the a11y concerns about patterns (heading levels, basically). Screen.Recording.2024-09-26.at.18.22.10-lww.mov(Please ignore the PX hardcoding for sizes on the files I've touched on the demo, it was for demo purposes and to show how it could be).
I believe what I am proposing will have the least amount of concessions font-size wise, but maybe I am missing something. |
I may be misunderstanding but I don't think these two suggestions are mutually exclusive? |
@carolinan, exactly. They are not mutually exclusive in the sense that they can be worked separately. But they both touch, in some capacity, the sizes of the headings and the XL. I see that there's already an effort in this PR to update the XL size and all heading sizes (h1 to h6), but they don't match the ones in Figma. That's why I brought it up for discussion, to see if we consider the other approach that will match what we have in Figma. |
Do the sizes in Figma now match this list? because I think this is the latest list from Bea: H1: 2.62rem |
So if the merge conflicts are solved, this can be merged? Because this task feels blocked by it: #423 |
I created a rebased version of this branch in #445. The reason it's a new PR is because I was not able to test in depth, and didn't want to mess up this branch in case I made errors. Hopefully this is helpful, so we can compare and contrast. |
@carolinan yes, Figma is in line with those sizes. Also, XL equals H2, and XXL equals H1 there. |
Description
This at least gets us onto a consistent system. There will be a few areas here and there we need to decide to fall in line with this system, or provide one-off styles. Those should be rare.
Screenshots