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

Fix heading sizes, to use custom sizes and match designs. #286

Closed
wants to merge 14 commits into from

Conversation

juanfra
Copy link
Member

@juanfra juanfra commented Sep 12, 2024

Description

Now that we have the custom heading sizes we need to fix the font sizes and HTML elements for most of the patterns, to match the designs.

Partial for #186

Patterns:

  • Clients section
  • Pricing two columns
  • Event schedule
  • Events – List with Call to Action
  • Grid with videos
  • Grid with categories
  • Hero Centered Heading
  • Hero podcast
  • Short heading and paragraph and image on the left
  • Services - Team photos
  • Two Headings and a List
  • Heading and paragraph with image on the right
  • Services - Subscriber only section
  • Events three columns with event images and titles

Screenshots

N/A

Testing Instructions

  • Create a page.
  • Add the following patterns modified in this PR.
  • Confirm that the headings are using the proper HTML elements (to improve a11y) and the same sizes as the designs.

@juanfra juanfra added the [Type] Enhancement A suggestion for improvement. label Sep 12, 2024
Copy link

github-actions bot commented Sep 12, 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: juanfra <[email protected]>
Co-authored-by: richtabor <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: beafialho <[email protected]>

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

@richtabor
Copy link
Member

I'm not particularly confident in this approach. What is the reasoning behind this again? Can we not use font sizes in their standard implementation?

@carolinan
Copy link
Contributor

I initially recommended using five font sizes where the heading sizes matches the presets.
The compromises was six sizes. Then later the sixth size was removed because the "t-shirt size" control only shows five.

Additional font sizes have been added during reviews, resulting in some elements having one-off sizes in the patterns, which makes the style variations / typography presets difficult to use.
One-off sizes is not a problem if they are limited to a few, like in a single pattern with a gigantic text.
It does become a problem when the sizes are needed to make an H2 look like an H1.

Last Friday I suggested that we either use the selectable font sizes for the headings, or add the heading sizes as custom CSS properties in theme.json.

There will always be compromises. I don't want to put words in @beafialho's mouth, so to say, but there has already been a number of compromises including the accessibility related changes, and features that are missing in the editor which has meant that some parts of the design has not been possible. It is important that @beafialho can still feel that the theme lives up to the vision and design.

@beafialho
Copy link
Contributor

Thank you, I appreciate your attention in wanting to maintain the original design hierarchy.

I tested this and my take, looking at the patterns without these custom sizes, I don't think we absolutely need this fix.

The change I made to the heading sizes in #231 already accounted that some of the patterns might look slightly different, but I don't think they look bad, especially because we have been using the correct heading levels for accessibility and applying some of the L / XL / XXL font size presets when necessary to improve their look.

That said however, if we do want to use this approach, the patterns look good to me as well 👍

@carolinan
Copy link
Contributor

The main issue is not with the look of these pattern, but that the heading sizes are not included in the font size presets.
H1 could have matched the largest font size preset, H6 the smallest but with uppercase, and so on.
Then the custom CSS variables would not be needed, and the user would be able to select from all the sizes.

@richtabor
Copy link
Member

"custom": {
			"fontSize": {
				"huge": "clamp(2.5rem, 4vw, 4.375rem)",
				"heading-1": "clamp(1.561rem, 1.2288rem + 1.661vw, 2.62rem)",
				"heading-2": "clamp(1.146rem, 0.9408rem + 1.026vw, 1.8rem)",
				"heading-3": "clamp(1.06rem, 0.878rem + 0.91vw, 1.64rem)",
				"heading-4": "clamp(0.984rem, 0.8222rem + 0.809vw, 1.5rem)",
				"heading-5": "clamp(0.875rem, 0.7574rem + 0.588vw, 1.25rem)",
				"heading-6": "0.875rem"
			}
		},

This will just confuse people. Why the theme is defining custom font sizes on top of the standard implementation. We should not do this.

It does become a problem when the sizes are needed to make an H2 look like an H1.

Why not use a block style variation for text styles?

@carolinan
Copy link
Contributor

Why not use a block style variation for text styles?

That is possible, and it may be a helpful addition to users 👍 But it wouldn't actually solve the problem with the large number of font sizes. The value still needs to be defined, and defined in one place, so that it can be updated from one place if needed.

@carolinan
Copy link
Contributor

carolinan commented Sep 15, 2024

I don't think a style variation should be used only to change a size. That would seem incredibly redundant to me, when there is a size option that could do the same, if only the presets and the heading sizes were the same.

@juanfra
Copy link
Member Author

juanfra commented Sep 16, 2024

To give more context about the reasoning behind this:

  • Most pattern designs have a main title using the size of an H1, and for a11y reasons, we can't use H1 on most. Using an H2, would make these titles more accessible, but the size is too small compared to the designs.
  • We have five font sizes (S, M, L, XL, XXL) that do not match the Headings. If we set an H2 with XL, it looks too small, and XXL is too large.

Thinking of a11y as one of the top priorities, the patterns' main titles should have an H2 element, but we needed to figure out how to handle the size (for the reasons described above) so that the patterns looked like they did in Figma.

That's why Carolina suggested using custom variables with the heading sizes that we could use in the code. This PR uses the sizes we introduced, so the H2s have the sizing of the Figma designs. From what I understand, the custom variables are not seen in the UI, and it could be invisible to many users.

I believe that if we don't want to go down this road, we should consider:

  • Making a pass on patterns and considering updating them, so that the titles use one of the sizes we have in the presets. In theory, the size should be smaller than H1, so that when the patterns are inserted in the context of a page with title, the hierarchy is respected.
  • OR review the heading sizes, but that was already checked here, so reverting the heading sizes will put us in the same situation with the headings that Bea fixed. So, I personally believe this is a no-go.

In my opinion, our main problem is that the size of the H2 doesn't match (or closely match) the presets we have. If we figure that out, we should be good without this. Because the problem is that we need to use H2's for a11y purposes, and that doesn't match the design.

I believe that having presets for headings only changing the size could be confusing for users, as they'll have three places competing for the same functionality. They'll be able to change the size from the level selector (H1, H2, .., H6), the sizes in the typography section, and the variation.

@beafialho
Copy link
Contributor

If we set an H2 with XL, it looks too small, and XXL is too large.

Just to make sure it's clear, as a compromise, this should be fine. I've been using these two presets in templates when the Heading 2 size is too small.

I strongly advocate that we do not change the heading sizes one more time. I have defined them so they look perfect in the default template and across the others. If we keep on changing them, we risk having things looking like this again, which I'd really like to avoid.

@beafialho
Copy link
Contributor

Making a pass on patterns and considering updating them, so that the titles use one of the sizes we have in the presets. In theory, the size should be smaller than H1, so that when the patterns are inserted in the context of a page with title, the hierarchy is respected.

This seems like the way to go to me 👍

@carolinan

This comment was marked as resolved.

@beafialho

This comment was marked as resolved.

@carolinan

This comment was marked as resolved.

@beafialho
Copy link
Contributor

Sorry, but I don't think I'm understand the problem. Could you exemplify with a recording or screenshot?

@carolinan
Copy link
Contributor

carolinan commented Sep 16, 2024

Sorry, but I don't think I'm understand the problem. Could you exemplify with a recording or screenshot?

If a user adds a pattern where there is a heading H2 that has the custom font size -H1, there is no interface, no setting, for the user to also add an H2 heading with the custom font size H1. They can't select that size for their H2.
So they can't create a custom section that matches the same design. Unless they realize they can just copy the pattern, of course.

#305 can resolve that, but it can also be overwhelming if all sizes are added there.

@beafialho
Copy link
Contributor

If a user adds a pattern where there is a heading H2 that has the custom font size -H1

So yet another reason not to add custom heading sizes, right? On the other hand, if a user adds a pattern where there is a heading 2 with the XL font size preset, they can remove that preset or pick another from the typography panel.

@carolinan
Copy link
Contributor

carolinan commented Sep 17, 2024

The XL font size is in the option though. So that is not a problem.
The H1 heading font size is not.


Do I understand correctly that the suggestion is to close this PR and not do these changes?

@beafialho
Copy link
Contributor

I'm not suggesting closing because I'm not sure what's the best solution.

Mostly trying to understand what the issue is and expressing my concern: that the font sizes have already been changed a few times, I've already gone through the heading sizes to get them to look just right, both in the posts, as well as in the patterns (using the correct headings and applying the presets as necessary).

If we want to add these custom sizes, I am fine with that as long as we do not move backwards on how things look across the theme.

@juanfra juanfra mentioned this pull request Sep 24, 2024
@richtabor
Copy link
Member

I'm working on another variation of this.

@juanfra juanfra mentioned this pull request Sep 26, 2024
@juanfra
Copy link
Member Author

juanfra commented Sep 26, 2024

Closing in favor of #422

@juanfra juanfra closed this Sep 26, 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