Skip to content

Add logo block#1275

Closed
aristath wants to merge 14 commits intoWordPress:masterfrom
aristath:fix/53247
Closed

Add logo block#1275
aristath wants to merge 14 commits intoWordPress:masterfrom
aristath:fix/53247

Conversation

@aristath
Copy link
Copy Markdown
Member

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


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.

@youknowriad
Copy link
Copy Markdown
Contributor

Can we enable the site-logo block in the same patch and add the required code? (I think to do so, there's a couple PHP files/webpack config to change and run npm run build:dev)

@aristath aristath changed the title Add REST-API changes required for the site-logo block Add logo block May 21, 2021
@aristath
Copy link
Copy Markdown
Member Author

Thank you @youknowriad 👍
Added the block and fixed a related phpunit test.

@youknowriad
Copy link
Copy Markdown
Contributor

@aristath looks like there are some failing phpunit tests here, any idea?

@aristath
Copy link
Copy Markdown
Member Author

Fixed the failing tests

@TimothyBJacobs
Copy link
Copy Markdown
Member

At a minimum the name for the theme mods option in the REST API should just be theme_mods. It also should be using set_theme_mod and probably get_theme_mod. But again, putting that in this endpoint is entirely incorrect. The capability is incorrect, it is using manage_options instead of edit_theme_options and conceptually is a mismatch as the theme mods API is an abstraction over the options API.

Theme mods should either be added to the themes endpoint, or more ideally have an endpoint to interact with registered settings in the customizer.

This is not suitable to be shipped in Core. If you aren't interested in making those endpoints, then I'd recommend looking at calling the customizer admin-ajax API. Ex: https://github.com/WordPress/gutenberg/blob/854a31f3edee98e8b16ef5f2d32abcdaa8b395b0/packages/edit-navigation/src/store/actions.js#L195

This also seems to have a different change of adding the stylesheet option to the settings endpoint. I'm not sure why this is here, but it can't ship. If you need the active theme get it from the themes endpoint. If you need to change themes, you can't just do that by updating the option. Theme switching needs to be added to the themes endpoint.

aristath added a commit to WordPress/gutenberg that referenced this pull request May 26, 2021
@aristath
Copy link
Copy Markdown
Member Author

The logo block was updated in WordPress/gutenberg#32229. Once a new minor Gutenberg release goes out I will update this PR with the new implementation (npm run build:dev doesn't pull from Gutenberg's trunk)

@aristath
Copy link
Copy Markdown
Member Author

For the sake of this PR I updated @wordpress/block-library to v3.2.2, cleaned-up previous files and updated the ones relevant to the site-logo block.
Tests are currently failing, but it should be OK once the block-library package gets updated in the master branch and this gets rebased.

@youknowriad
Copy link
Copy Markdown
Contributor

Packages are now up to date on trunk, would you mind refreshing this PR?

@youknowriad
Copy link
Copy Markdown
Contributor

The block seems to work in my testing. Any idea about the failing tests? Is it still the action unhooking thing?

@aristath
Copy link
Copy Markdown
Member Author

aristath commented Jun 1, 2021

looking into the failing tests now 👍

@aristath
Copy link
Copy Markdown
Member Author

aristath commented Jun 1, 2021

All tests now pass. I needed to tweak the sync hook a bit, and a PR backporting these changes to Gutenberg was submitted in WordPress/gutenberg#32370

@youknowriad
Copy link
Copy Markdown
Contributor

Ok makes sense. I guess this means we should wait for the next package update to land this PR to avoid having npm run build override your changes.

@aristath
Copy link
Copy Markdown
Member Author

aristath commented Jun 7, 2021

Updated, should be OK to merge now 👍

@youknowriad
Copy link
Copy Markdown
Contributor

committed in https://core.trac.wordpress.org/changeset/51091

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.

4 participants