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

Block Themes: Add Infrastructure #1796

Closed
wants to merge 43 commits into from
Closed

Block Themes: Add Infrastructure #1796

wants to merge 43 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Nov 2, 2021

Work in progress. Attempts to add the required infrastructure to render block-based themes to Core.

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

TODO

  • Add Template Part CPT and TP area taxonomy registration, unless we decide that it's not strictly needed to render block themes, and defer to a separate PR. Note that I've reverted for now (437a16a) since it caused errors in wp-admin.
  • Add minified CSS to Template Part block.
  • Add unit test coverage (carry over from Gutenberg).
  • Remove gutenberg_ prefixes and _gutenberg_ infixes.
  • Add/update PHPDocs to cover added/changed functionality; add @since items where needed.

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
Contributor

I've merged template-part utils and template utils because the template-utils already worked for both. We just missed some functions.

@youknowriad
Copy link
Contributor

Add minified CSS to Template Part block.

What is this item about @ockham I think there's no stylesheet for this block if I'm not wrong?

@youknowriad
Copy link
Contributor

Ok, I think this one is actually ready, the only issue is the manual modifications to the template part blocks which is breaking some tests (unit, e2e tests, npm).

*
* @return array Template.
*/
function _get_block_templates_files( $template_type ) {

Choose a reason for hiding this comment

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

Should this be _get_block_template_files? (no plural in templates)

Copy link
Contributor

Choose a reason for hiding this comment

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

I hesitated but I choice "templates" because it's multiple templates while template-files could mean mutiple files for a single template. Not entirely confident with my choice, that said, it's not very important because it's a private function.

* Should retrieve the template from the theme files.
*/
function test_get_block_template_from_file() {
$this->markTestIncomplete();

Choose a reason for hiding this comment

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

Why skipped this? I'm having the same issue in the GB PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here It's skipped because there's no FSE theme we can active on Core yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WP 5.9 will include Twenty Twenty Two though 🤔 I guess we should file an issue or ticket somewhere to re-enable these tests once we have that theme available in Core.

@noisysocks
Copy link
Member

Ok, I think this one is actually ready, the only issue is the manual modifications to the template part blocks which is breaking some tests (unit, e2e tests, npm).

It's a bit of a chicken and egg problem because #1804 which updates the blocks properly depends on this PR 🙂

I guess we will need to just skip the tests or commit this with failing tests and then immediately follow up with commits for https://core.trac.wordpress.org/ticket/54336 and https://core.trac.wordpress.org/ticket/54337.

@youknowriad
Copy link
Contributor

@noisysocks yeah, I'm also fine if we include this patch in the packages update one, whatever works for you.

noisysocks and others added 16 commits November 9, 2021 09:43
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@noisysocks
Copy link
Member

Thanks @Bernie! I'll rebase and commit this.

@noisysocks
Copy link
Member

Going to ignore the lint failures as they are failing on trunk too.

https://wordpress.slack.com/archives/C02RQBWTW/p1636411775055600

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