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

add: spacing styles for heading with annotation style in dust theme #636

Closed
wants to merge 1 commit into from

Conversation

troychaplin
Copy link
Contributor

Description

Updated spacing as discussed in #605.

I set the bottom padding to 0.15rem instead of the originally suggested 0.2 based on a heading set in all caps still appearing to have more bottom space than the top.

Screenshots

test-2 test-1

@troychaplin troychaplin linked an issue Oct 26, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Oct 26, 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 <areziaal@git.wordpress.org>
Co-authored-by: juanfra <juanfra@git.wordpress.org>
Co-authored-by: rejaulalomkhan <rejaulalomkhan@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>

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

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.

@troychaplin troychaplin requested review from carolinan and juanfra and removed request for carolinan October 26, 2024 00:55
@rejaulalomkhan
Copy link
Contributor

@troychaplin Thanks for your valuable PR.
Perfect, thanks for addressing the spacing issue as discussed in #605. The adjustment to 0.15rem for bottom padding makes sense given the heading's appearance. Nice work.

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.

Thank you, Troy!

This would be initially fixing the annotation padding for the dusk variation. The problem is that once saved, the CSS will be persistent and if you change the style variation then it'll affect how the annotation is viewed. The result would be creating this new problem:

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

@@ -60,6 +60,7 @@
"letterSpacing": "-0.18px",
"lineHeight": "1.5"
},
"css": ".wp-block-heading.is-style-text-annotation {padding-bottom:0.15rem;}",
Copy link
Member

Choose a reason for hiding this comment

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

I still see a bit more spacing at the bottom. In my testing I believe approaching 0.1rem makes it look better. I'll defer to @beafialho to decide the value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll make any additional changes necessary once we hear back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juanfra stepped away for a but, just following up on a few items. Are we still waiting for confirmation on this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@troychaplin thanks for coming back to this one. Unfortunately, as mentioned in the review, the PR will have the side effect of keeping/persisting the CSS once saved, affecting styles when changing variation. We need to find a solution that doesn't have this type of side effects. If we find it, we could confirm the padding-bottom of the annotation style for that variation in particular.

Copy link
Contributor Author

@troychaplin troychaplin Nov 4, 2024

Choose a reason for hiding this comment

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

@juanfra it sounds like it might be best to just close this PR and remove my name from the issue. I will still take some time to look at other solutions, but best to leave it unassigned in case someone else wants to take a look as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thank you Troy!

@troychaplin
Copy link
Contributor Author

Closing pull request to pave the way for someone else who may have a new or better approach to solving this issue

@troychaplin troychaplin closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants