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

Don't use fluid layout value in typography calculation. #5006

Closed

Conversation

tellthemachines
Copy link
Contributor

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

Adds the PHP changes from WordPress/gutenberg#53551 and WordPress/gutenberg#53554 to core.

To test: using Twenty Twenty Three or another block theme with fluid typography enabled, change the value of settings.layout.wideSize in theme.json to use a fluid value such as clamp(1000px, 85vw, 2000px). Check that font sizes remain fluid in the front end. (For the editor part, an npm package update will be required; that will be done separately.)


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Works for me. Thank you!

Before

2023-08-14.15.55.43.mp4

After

2023-08-14.15.56.04.mp4

Aside from the minor PHP lint warning (Expected 1 space before "?"; 2 found), I think the failing PHP test Tests_Theme_ThemeDir::test_theme_list is related. It's expecting Block Theme Child Theme With Fluid Layout in the array.

@@ -517,7 +517,7 @@ function wp_get_typography_font_size_value( $preset, $should_use_fluid_typograph
: array();

// Defaults.
$default_maximum_viewport_width = isset( $layout_settings['wideSize'] ) ? $layout_settings['wideSize'] : '1600px';
$default_maximum_viewport_width = isset( $layout_settings['wideSize'] ) && ! empty( wp_get_typography_value_and_unit( $layout_settings['wideSize'] ) ) ? $layout_settings['wideSize'] : '1600px';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$default_maximum_viewport_width = isset( $layout_settings['wideSize'] ) && ! empty( wp_get_typography_value_and_unit( $layout_settings['wideSize'] ) ) ? $layout_settings['wideSize'] : '1600px';
$default_maximum_viewport_width = isset( $layout_settings['wideSize'] ) && ! empty( wp_get_typography_value_and_unit( $layout_settings['wideSize'] ) ) ? $layout_settings['wideSize'] : '1600px';

Fix PHPCS notice

@tellthemachines
Copy link
Contributor Author

Thanks for the reviews folks! Looks like all the tests are passing now.

@tellthemachines
Copy link
Contributor Author

Committed to trunk in r56503; leaving this PR open for commit to the 6.3 release branch.

@tellthemachines tellthemachines changed the base branch from trunk to 6.3 September 7, 2023 05:25
@tellthemachines tellthemachines changed the base branch from 6.3 to trunk September 7, 2023 05:25
@tellthemachines
Copy link
Contributor Author

Closing as this was committed to the 6.3 branch in https://core.trac.wordpress.org/changeset/56737

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