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

Remove get_default_block_editor_settings from theme JSON resolver #3902

Conversation

Mamaduka
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/57547
Gutenberg PR: WordPress/gutenberg#46112


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

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@Mamaduka Two small bits of feedback on the production code.

Can you add unit test coverage for the change please? Particularly a simple test for the new function. The get_default_block_editor_settings() change should already be covered by existing tests.

src/wp-includes/block-editor.php Outdated Show resolved Hide resolved
src/wp-includes/block-editor.php Outdated Show resolved Hide resolved
@spacedmonkey
Copy link
Member

Before

Screenshot 2023-01-25 at 09 13 03

After

Screenshot 2023-01-25 at 09 12 39

Performance improvement 👍

@Mamaduka
Copy link
Member Author

Updated the function name and added simple unit tests.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@Mamaduka One last tiny bit of feedback, other than that this is good to go.

tests/phpunit/tests/blocks/editor.php Outdated Show resolved Hide resolved
tests/phpunit/tests/blocks/editor.php Show resolved Hide resolved
@Mamaduka
Copy link
Member Author

Good catch. Thank you, @felixarntz!

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.

Thanks @Mamaduka, I've left some feedback regarding the unit test in a PR review.

tests/phpunit/tests/blocks/editor.php Outdated Show resolved Hide resolved
tests/phpunit/tests/blocks/editor.php Outdated Show resolved Hide resolved
@Mamaduka
Copy link
Member Author

Thank you, @mukeshpanchal27!

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @Mamaduka, LGTM!

@felixarntz
Copy link
Member

Committed in https://core.trac.wordpress.org/changeset/55146 🎉

@felixarntz felixarntz closed this Jan 26, 2023
@Mamaduka Mamaduka deleted the backport/legacy-theme-supports-settings branch May 9, 2023 09:19
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.

5 participants