-
Notifications
You must be signed in to change notification settings - Fork 104
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
Conversation
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'm not particularly confident in this approach. What is the reasoning behind this again? Can we not use font sizes in their standard implementation? |
I initially recommended using five font sizes where the heading sizes matches the presets. 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. 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. |
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 That said however, if we do want to use this approach, the patterns look good to me as well 👍 |
The main issue is not with the look of these pattern, but that the heading sizes are not included in the font size presets. |
This will just confuse people. Why the theme is defining custom font sizes on top of the standard implementation. We should not do this.
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. |
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. |
To give more context about the reasoning behind this:
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:
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. |
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. |
This seems like the way to go to me 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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. #305 can resolve that, but it can also be overwhelming if all sizes are added there. |
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. |
The XL font size is in the option though. So that is not a problem. Do I understand correctly that the suggestion is to close this PR and not do these changes? |
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. |
I'm working on another variation of this. |
Closing in favor of #422 |
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:
Screenshots
N/A
Testing Instructions