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.

Sign up for GitHub

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

Jump to bottom

Backport spacing presets #3255

Closed
wants to merge 18 commits into from
Closed

Backport spacing presets #3255

wants to merge 18 commits into from

Conversation

glendaviesnz
Copy link

@glendaviesnz glendaviesnz commented Sep 15, 2022 •
edited by ockham
Loading

❗ BLOCKED by #3204 Edit: #3204 has been merged.

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

@glendaviesnz glendaviesnz marked this pull request as ready for review September 15, 2022 02:26
@andrewserong andrewserong mentioned this pull request Sep 15, 2022
Closed
37 tasks
costdev
costdev requested changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for handling this PR @glendaviesnz! I've left some thoughts below.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
@costdev costdev mentioned this pull request Sep 15, 2022
Closed
2 tasks
costdev
costdev reviewed Sep 16, 2022
View reviewed changes
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
costdev
costdev requested changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the updates @glendaviesnz! A few more thoughts and a comment on the spacingScale discussion from my last review.

@glendaviesnz
Copy link
Author

glendaviesnz commented Sep 19, 2022 •
edited
Loading

@costdev fyi there is now some discussion going about whether the t-shirt size style labels will be dropped for 6.1, so need to wait for a decision on that and will then update this PR accordingly.

@costdev
Copy link
Contributor

costdev commented Sep 19, 2022

Thanks for the update @glendaviesnz!

@glendaviesnz
Copy link
Author

spacingScale in this context is the name of the test method's argument rather than the method being tested, which should match $spacing_scale in the test method's signature.

🤦 so it is, doh! fixed.

@glendaviesnz glendaviesnz mentioned this pull request Sep 20, 2022
Merged
@glendaviesnz
Copy link
Author

I think this should be good to merge, pending @costdev double checking the switch to (string) casting

costdev
costdev approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks @glendaviesnz

mukeshpanchal27
mukeshpanchal27 reviewed Sep 21, 2022
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

LGTM. left minor document change.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@audrasjb @mukeshpanchal27
small return mention improvement
b0b1d6d 
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@audrasjb audrasjb self-assigned this Sep 21, 2022
@audrasjb
Copy link
Contributor

Committed in https://core.trac.wordpress.org/changeset/54272

@audrasjb audrasjb closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewserong andrewserong andrewserong left review comments

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 left review comments

@costdev costdev costdev approved these changes

Assignees

@audrasjb audrasjb

Labels
None yet
Projects
No open projects
WordPress 6.1 Editor Tasks
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@glendaviesnz @costdev @audrasjb @mukeshpanchal27 @andrewserong