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

Typography: minimum font size constraint bugfix #3489

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Oct 19, 2022

This PR backports the typography bugfix from WordPress/gutenberg#44993

It:

  • adds default minimum font size limits so that min font size, where provided, does not become smaller than 14px/0.875rem/0.875em.
  • For font sizes of < 14px that have no defined minimum sizes, uses the font size to set the floor of the clamp() value.

Trac ticket: https://core.trac.wordpress.org/ticket/56467

Gutenberg bug issue: WordPress/gutenberg#44957
Gutenberg PR: WordPress/gutenberg#44993

Reasons for backporting:

This bugfix prevents converting existing small font sizes to clamp values that will reduce their font size even further in narrow widths. It therefore improves backwards compatibility and accessibility.

Testing

See WordPress/gutenberg#44993 for comprehensive testing instructions.

❗ This only affects the frontend.

Enable TT3 theme

In the editor reduce a text block's font size to 10px

Screen Shot 2022-10-19 at 10 54 49 am

Publish and inspect the element on the frontend. The font-size should be font-size: clamp(10px, 0.625rem + ((1vw - 7.68px) * 0.601), 15px);

Edit the theme.json to include a font size preset whose min font size is <14px/0.875rem

				{
					"fluid": {
						"min": "0.3rem",
						"max": "40px"
					},
					"size": "1rem",
					"slug": "small"
				},

The generated preset's min value in the clamp() function should not be <14px/0.875rem:

--wp--preset--font-size--small: clamp(0.875rem, 0.875rem + ((1vw - 0.48rem) * 3.125), 40px);

- Adds default minimum font size limits so that min font size, where provided, does not become smaller than 14px/0.875rem/0.875em. The consequence will be that all fluid font sizes will be no smaller than 14px/0.875rem/0.875em in any viewport width.
- For font sizes of < 14px that have no defined minimum sizes, use the font size to set the floor of the clamp() value.
@ramonjd ramonjd changed the title Backporting changes in https://github.com/WordPress/gutenberg/pull/44993 Typography: minimum font size constraint bugfix Oct 19, 2022
@ramonjd
Copy link
Member Author

ramonjd commented Oct 19, 2022

cc @tellthemachines @noisysocks @andrewserong to check things out and make sure there's nothing missing 😄

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Code looks good to me! All the PHP changes from WordPress/gutenberg#44993 are here.

Tested on Chrome, Firefox and Safari with different browser settings for font size, as well as browser zoom. Everything works as expected ✅

Copy link

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing well for me, too, thanks for getting this backport up 👍

✅ Individual custom font-sizes < 14px are allowed, and the value is used as the floor of the clamp() calculation
✅ For font sizes greater than 14px, the floor of 14px is used for the min value of the clamp() calculation
min values in font size presets correctly have a floor of 14px or the appropriate em/rem equivalent of 0.875

LGTM! ✨

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

[Committer double-signoff]

Confirming that this tests as described, and prevents text formatted with fluid font sizes to go tiny ✅

Also confirming that the PHP code changes match the Gutenberg PR that was already approved for backporting ✅

LGTM! Thank you 😊

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in r54646, backported to the 6.1 branch in r54647.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 19, 2022

I'm very grateful for the quick reviews and commit. Thank you!

@Mamaduka
Copy link
Member

@SergeyBiryukov, do you know if r54647 was reverted from 6.1 branch?

@Mamaduka
Copy link
Member

Good, the commit hasn't been reverted. We'll ship the client-side code in 6.1.1.

@SergeyBiryukov
Copy link
Member

SergeyBiryukov commented Nov 10, 2022

Good, the commit hasn't been reverted. We'll ship the client-side code in 6.1.1.

Yeah, I remember a Slack discussion about this with the consensus to revert, but it looks like the code is still in the branch, so I guess the revert was missed somehow 😅

@Mamaduka
Copy link
Member

That saves us work for the minor release 😄

@ramonjd
Copy link
Member Author

ramonjd commented Nov 10, 2022

Thanks folks. There are (potentially) incoming changes for 6.1.1 stemming from WordPress/gutenberg#45536... I'll create a core patch and see how far we get 😄

@Mamaduka
Copy link
Member

Thank you, @ramonjd. I've already cherry-picked WordPress/gutenberg#44993 for 6.1.1.

@ramonjd
Copy link
Member Author

ramonjd commented Nov 10, 2022

PHP backport for WordPress/gutenberg#45536 here:

This updates the logic and tests since the changes in WordPress/gutenberg#44993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6 participants