Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

update: remove letter spacing from heading 5 in afternoon style #633

Merged

Conversation

troychaplin
Copy link
Contributor

Description

Remove letter spacing from heading as per comment on #629

Screenshots

Before

before

After

after

Copy link

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 Oct 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: troychaplin <[email protected]>
Co-authored-by: juanfra <[email protected]>
Co-authored-by: beafialho <[email protected]>
Co-authored-by: sabernhardt <[email protected]>
Co-authored-by: NidhiDhandhukiya74 <[email protected]>

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

@beafialho beafialho requested a review from juanfra October 25, 2024 13:22
@beafialho
Copy link
Contributor

@troychaplin could you please change the title of this PR? I confused it with making changes to the header.

Also, are you changing theme.json, as in, this will be reflected in all variations? My suggestion was this change should only happen in Afternoon and its respective typography preset, not all the others.

@troychaplin
Copy link
Contributor Author

troychaplin commented Oct 25, 2024

@troychaplin could you please change the title of this PR? I confused it with making changes to the header.

Also, are you changing theme.json, as in, this will be reflected in all variations? My suggestion was this change should only happen in Afternoon and its respective typography preset, not all the others.

There was no specific reference in the files that relate directly to Afternoon, in theme.json was where I found the reference and tested. I can post before and after screenshots of each style when I get home

@juanfra juanfra linked an issue Oct 25, 2024 that may be closed by this pull request
@juanfra
Copy link
Member

juanfra commented Oct 25, 2024

Thanks for working on this @troychaplin - As @beafialho mentioned, the intention is only to remove the letter spacing in the Afternoon style variation.

Removing the letter spacing for the h5 in the theme.json file will affect all variations. And that's not what we want. The variations "inherit" what was defined in the theme.json.

So, I would try a different approach for this. As we only want to remove the letter spacing on the h5 of the afternoon variation, you could set the letter spacing there to 0/initial. That will keep the desired letter spacing for all other variations (as defined in the theme.json) but only remove it from "Afternoon".

Please remember that the variations have a specific typography preset associated, so you'll also need to make the same adjustment there. For "Afternoon", "typography preset 3" is the associated one.

@troychaplin troychaplin changed the title update: header 5 letter spacing update: remove letter spacing from heading 5 in afternoon style Oct 25, 2024
@troychaplin
Copy link
Contributor Author

@juanfra is there a preference / standard to setting letterSpacing as 0, or normal? Adding either one to the h5 in the primary afternoon.json file will fix the issue. Also, there is no reference to specific heading levels in in the typography-preset-3.json file

@troychaplin
Copy link
Contributor Author

@juanfra before this gets merged in there's still an outstanding question that is waiting comment from @beafialho

@beafialho
Copy link
Contributor

beafialho commented Oct 26, 2024

@troychaplin apologies for the extra work, but would you mind posting screenshots/recordings of the before and after this update in all variations please? With so many reviews on top of previous edits, I worry I may have something not looking like your PR.

@troychaplin
Copy link
Contributor Author

@troychaplin apologies for the extra work, but would you mind posting screenshots/recordings of the before and after this update in all variations please? With so many reviews on top of previous edits, I worry I may have something not looking like your PR.

There's no before and after to post for all styles, the change only impacts the afternoon styles, screenshots added below. In reference to the other item I saw that I think might need a change, I will post a more detailed follow up to my comment in the issue

Before After
afternoon-before afternoon-after

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks @troychaplin! Can you pleas also update the same value in typography-preset-3.json so that we have parity?

This way, when the style variation is selected, it'll also automagically select the style variation.

@juanfra
Copy link
Member

juanfra commented Oct 28, 2024

You'll first need to rebase and merge trunk to your branch to see the h5 reference in typography-preset-3.json.

@troychaplin
Copy link
Contributor Author

Thanks @troychaplin! Can you pleas also update the same value in typography-preset-3.json so that we have parity?

This way, when the style variation is selected, it'll also automagically select the style variation.

No prob, changes pushed

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks @troychaplin - Looks good to me 🚀

Screen.Recording.2024-10-28.at.17.35.19.mov

@juanfra juanfra merged commit b57f165 into WordPress:trunk Oct 28, 2024
5 checks passed
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.

Heading h6 is looking bigger than the heading h5.
3 participants