Skip to content

Remove get_default_block_editor_settings from theme JSON resolver#3902

Closed
Mamaduka wants to merge 6 commits intoWordPress:trunkfrom
Mamaduka:backport/legacy-theme-supports-settings
Closed

Remove get_default_block_editor_settings from theme JSON resolver#3902
Mamaduka wants to merge 6 commits intoWordPress:trunkfrom
Mamaduka:backport/legacy-theme-supports-settings

Conversation

@Mamaduka
Copy link
Copy Markdown
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
Copy Markdown
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.

@spacedmonkey
Copy link
Copy Markdown
Member

Before

Screenshot 2023-01-25 at 09 13 03

After

Screenshot 2023-01-25 at 09 12 39

Performance improvement 👍

@Mamaduka
Copy link
Copy Markdown
Member Author

Updated the function name and added simple unit tests.

Copy link
Copy Markdown
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.

@Mamaduka
Copy link
Copy Markdown
Member Author

Good catch. Thank you, @felixarntz!

Copy link
Copy Markdown
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.

@Mamaduka
Copy link
Copy Markdown
Member Author

Thank you, @mukeshpanchal27!

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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