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

REST API: Expose blocks registered on the server #317

Closed
wants to merge 37 commits into from

Conversation

spacedmonkey
Copy link
Member

Ported from WordPress/gutenberg#21065

Related post:
https://make.wordpress.org/core/2019/05/27/the-block-registration-api-status-update/
Related GitHub issue:
WordPress/gutenberg#2751
Related Block Registration API technical proposal with all the necessary details is available at:
https://github.com/WordPress/gutenberg/blob/master/docs/rfc/block-registration.md

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


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.

@TimothyBJacobs
Copy link
Member

We need to add @ticket annotations for the tests.

I'm also going to hold off on committing this until the Gutenberg merge happens for 5.5.

@spacedmonkey
Copy link
Member Author

We need to add @ticket annotations for the tests.

I'm also going to hold off on committing this until the Gutenberg merge happens for 5.5.

Added comment for tickets and version numbers.

@TimothyBJacobs
Copy link
Member

I added some comments, I'd also like to see a test that covers all the different fields. There is a test that iterates over all the properties, but only a few are defined. I'd like to see something like register_block_type( 'test', [ /* all the args */ ] )that then checks for each property with an expected value. Not repeating the logic from the controller itself, but asserting against a static string ( or object, array, etc... ). See the themes controller tests for instance.

@spacedmonkey
Copy link
Member Author

I added some comments, I'd also like to see a test that covers all the different fields. There is a test that iterates over all the properties, but only a few are defined. I'd like to see something like register_block_type( 'test', [ /* all the args */ ] )that then checks for each property with an expected value. Not repeating the logic from the controller itself, but asserting against a static string ( or object, array, etc... ). See the themes controller tests for instance.

I am not sure where tests for register_block_type start and the controller tests here. Sounds like you want to repeated logic. Not sure I see the value of this.

@TimothyBJacobs
Copy link
Member

I am not sure where tests for register_block_type start and the controller tests here. Sounds like you want to repeated logic. Not sure I see the value of this.

The majority of our controllers have tests that are covered by other areas, yet they are still important so we make sure everything is output as expected. I think the themes controller is a great example of this.

spacedmonkey and others added 11 commits June 25, 2020 18:59
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
spacedmonkey and others added 22 commits June 25, 2020 19:00
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Timothy Jacobs <timothy@ironbounddesigns.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
@TimothyBJacobs
Copy link
Member

Merged in 5b90ea4.

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