Skip to content
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

Closed
wants to merge 20 commits into from
Closed

Improved type system #422

wants to merge 20 commits into from

Conversation

richtabor
Copy link
Member

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.

  • Reduces the dependency on custom font size variables.
  • Adds a bit more consistency across typography.
  • Removes unnecessary typography attributes applied to blocks throughout patterns.

Screenshots

CleanShot 2024-09-25 at 17 14 56

@richtabor richtabor added the [Type] Enhancement A suggestion for improvement. label Sep 25, 2024
@richtabor richtabor self-assigned this Sep 25, 2024
Copy link

github-actions bot commented Sep 25, 2024

Preview changes

You 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.
⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

Copy link

github-actions bot commented Sep 25, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: richtabor <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@carolinan
Copy link
Contributor

I am not comfortable with these changes as these are changes that @beafialho has said no to repeatedly.

@carolinan
Copy link
Contributor

Does this PR closely match what I originally suggested after listing the font sizes I found in Figma? Yes.
Does it closely match what I have suggested during development? 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.

@jasmussen
Copy link
Contributor

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.

@carolinan
Copy link
Contributor

carolinan commented Sep 26, 2024

typography-preset-x1.json cleanup -This I feel that I can approve and is even necessary.
dusk, afternoon, midnight -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,
patterns/cta-heading-search.php
patterns/footer-columns.php
The biggest difference with this change is how the two sizes change depending on the browser width, where the new size is much larger on tablets. I feel that I can approve this, but needs a designers viewpoint.

Old size above, new size below:

image

patterns/cta-grid-products-link.php,
patterns/intro-short-heading-image.php
This just seems to be a cleanup, no problem approving this.

patterns/heading-and-paragraph-with-image.php
patterns/hero-full-width-image.php,
patterns/overlap-images-and-paragraph.php
I don't think the size should be changed, not without motivation and a final design review from Bea.

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
The problem is the dismissal of Bea's wishes to keep the separate heading sizes in addition to the presets.

A compromise would be to have them all in the font size control and not limit the control to the t-shirt size buttons.

@juanfra
Copy link
Member

juanfra commented Sep 26, 2024

I believe that once we merge this one (with whatever naming we decide), we need to:

  • Modify the XXL with the size of the H1.
  • Modify the XL with the size of the H2.

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 XL, and XXL matching the sizes of H2 and H1 respectively will help us address all the a11y concerns that were raised, and respecting all the heading sizes that are in Figma for all the patterns. And again, people will have control of any of these sizes in the UI.

XL matching the size of H2, and XXL matching the size of H1 is how things were done in TT4, and it looks solid.

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.

I chose large there, as the tagline in the design is 20px, and it was the closest. The tagline I used as a reference is the one in the footer (in Figma, the footer right after the "Other posts") section.

@richtabor
Copy link
Member Author

If we go that road, I believe this PR won't be necessary.

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.

@jasmussen
Copy link
Contributor

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:

Screenshot 2024-09-26 at 17 04 46

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:

image

Branch:

image

Figma:

image

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:

Fleurs is a flower delivery and subscription business. Based in the EU, our mission is not only to deliver stunning flower arrangements across but also foster knowledge and enthusiasm on the gift of nature: flowers.

Removing "beautiful".

Most of the patterns look virtually identical. I did get a validation issue with "Content location and link":

Screenshot 2024-09-26 at 17 29 32

The "Hero book" looks a bit better in trunk and Figma than here. Trunk:

Screenshot 2024-09-26 at 17 30 51

Branch:

image

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:

image

The first paragraph is a bit bigger in this branch:

image

IMO both look good, but Figma has it smaller:

image

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:

  • Remove "beautiful" from the banner with description and grid pattern to avoid the widow.
  • Consider readding the right padding in Hero book.
  • Consider whether the first paragraph in "hero full width image" should be bigger or not.

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.

@richtabor
Copy link
Member Author

richtabor commented Sep 26, 2024

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 ☝️:

CleanShot 2024-09-26 at 11 43 01

CleanShot 2024-09-26 at 11 44 38

CleanShot 2024-09-26 at 11 46 43

Copy link
Contributor

@jasmussen jasmussen left a 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.

@juanfra
Copy link
Member

juanfra commented Sep 26, 2024

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

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).

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.

I believe what I am proposing will have the least amount of concessions font-size wise, but maybe I am missing something.

@carolinan
Copy link
Contributor

I may be misunderstanding but I don't think these two suggestions are mutually exclusive?

@juanfra
Copy link
Member

juanfra commented Sep 27, 2024

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.

@carolinan
Copy link
Contributor

Do the sizes in Figma now match this list? because I think this is the latest list from Bea:

H1: 2.62rem
H2: 1.8rem
H3: 1.64rem
H4: 1.5rem
H5: 1.25rem
H6: 0.875rem (Bold, uppercase, 1.4px letter spacing)

#231

@carolinan
Copy link
Contributor

So if the merge conflicts are solved, this can be merged? Because this task feels blocked by it: #423

@jasmussen jasmussen mentioned this pull request Sep 27, 2024
@jasmussen
Copy link
Contributor

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.

@juanfra
Copy link
Member

juanfra commented Sep 27, 2024

Do the sizes in Figma now match this list? because I think this is the latest list from Bea:

@carolinan yes, Figma is in line with those sizes. Also, XL equals H2, and XXL equals H1 there.

Screenshot 2024-09-27 at 16 43 35

@richtabor richtabor closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants