Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Moved sizing opinions from home to theme.json #94

Closed
wants to merge 1 commit into from

Conversation

pbking
Copy link
Collaborator

@pbking pbking commented Aug 23, 2022

This change increases the font weight of H2 and H3 to 400. The only places I've noticed it's used is in the home template and both of those instances were increasing the font weight so I suggest we do that in theme.json instead.

The 'Latest Posts' text was an H3 customized to nearly the same values assigned to H6 so i just changed it to an H6.

Before:

image

image

After:
image

image

Partially addresses #84

@carolinan
Copy link
Collaborator

Please never change heading levels because you need to match a style :)
Using the correct heading levels are important for those who navigate via heading levels, like some screen reader users.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The font-weight changes are great, the more we can move out of the markup, the better!

In regards to the Latest Posts heading, I agree with @carolinan that it should remain a h3, but then this makes the styling tricky. It looks like we either need to keep the styling in the markup for this or change the h3 styles. Or perhaps we could remove this heading, as suggested here?

@carolinan
Copy link
Collaborator

Yeah I agree with removing the texts that are above the posts, and placing the query pagination and the separator below.

@madhusudhand
Copy link
Member

madhusudhand commented Aug 24, 2022

I see one case where the sizing needs to be added in the markup.
For post title, even though it is given h4, the font-size is derived from core/post-title because of the higher specificity.

<!-- wp:post-title {"level":4,"isLink":true,"style":{"spacing":{"margin":{"top":"var(--wp--preset--spacing--60)","bottom":"var(--wp--preset--spacing--60)"}}}} /-->

This needs fontSize defined in the template to match the figma.

Current Expected
image image

Update: #109 fixes the issue.

@mikachan
Copy link
Member

I completely forgot this PR was ongoing when I created this one: #111

Happy for this one to be preferred!

@pbking
Copy link
Collaborator Author

pbking commented Aug 24, 2022

Nah, I'll close this one in deference to #111 and add any comments to that one.

@pbking pbking closed this Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants