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: Remove theme attributes from block templates #172

Closed
wants to merge 8 commits into from

Conversation

jeyip
Copy link
Collaborator

@jeyip jeyip commented Jan 19, 2021

Changes proposed in this Pull Request:

Remove theme attributes from block-theme block templates

Context:

This PR was created as a follow-up to WordPress/gutenberg#28088. See the aforementioned PR description and code changes for more context.

Discussion:

  1. What this means for theme designers is that, when creating block templates, the theme attribute can be omitted from block templates (like we've done in this PR). The logic for the Gutenberg full site editor will automatically add a theme attribute that matches the current, activated theme's stylesheet.
  2. New sites created for full site editing will not load template parts correctly until Template Parts: Fix loading issue gutenberg#28088 and Avoid using auto-drafts for theme templates and template parts gutenberg#27910 are added to a release.

Testing:

  • Apply this PR
  • Run Gutenberg locally (see instructions here)
  • Install the tt1-blocks theme by referencing it in .wp-env.override.json
      // .wp-env.override.json
    
      // Replace ~/work/theme-experiments/tt1-blocks with 
      // the path to your locally installed tt1-blocks theme
      {
           "mappings": {
               "wp-content/themes/tt1-blocks": "~/work/themes/tt1-blocks",
           }
      }```
  • npx wp-env start --update
  • Activate the tt1-blocks theme
  • Open the front end and the site editor. FSE templates and template parts should load without issue.
  • Reinstall the tt1-blocks theme in a subdirectory by modifying .wp-env.override.json
       // .wp-env.override.json
      
       // Add a test subdirectory in the mapping
       {
            "mappings": {
                "wp-content/themes/test/tt1-blocks": "~/work/themes/tt1-blocks",
            }
       }```
  • npx wp-env start --update
  • Activate tt1-blocks.
  • Open the front end and the site editor. FSE templates and template parts should still load without issue.

@jeyip jeyip added the block-based theme A theme using HTML templates label Jan 19, 2021
@jeyip jeyip self-assigned this Jan 19, 2021
@jeyip jeyip force-pushed the update/block-template-theme-attributes branch from 9a0db89 to ddc4bf4 Compare January 19, 2021 18:44
@jeyip jeyip force-pushed the update/block-template-theme-attributes branch from ddc4bf4 to f0047ba Compare January 19, 2021 18:46
@jeyip jeyip requested review from kjellr and youknowriad January 19, 2021 20:27
@jffng jffng linked an issue Jan 19, 2021 that may be closed by this pull request
@jeyip jeyip requested a review from jffng January 19, 2021 21:54
@pattonwebz
Copy link
Member

I'm having a tough time understanding what problem this PR solves or if there might be alternative approaches.

It's the problem that when templates are created and then the theme is moved to a subdirectory and reactivated they aren't available in the reactivated theme?

@jeyip
Copy link
Collaborator Author

jeyip commented Jan 20, 2021

I'm having a tough time understanding what problem this PR solves or if there might be alternative approaches.

I'll update the PR description tomorrow to try and make the problem more accessible 👍

It's the problem that when templates are created and then the theme is moved to a subdirectory and reactivated they aren't available in the reactivated theme?

That's on the right track. What happens is that when the theme is initially created in or moved to any subdirectory, the template parts defined in theme-provided block templates won't load in the Full Site Editor.

The issue will look something like this:

101551397-33717400-39b1-11eb-8e31-91cfa8cd745d

Example

Take the front-page.html block template for example. It has a serialized template part that looks like this:

<!-- wp:template-part {"slug":"header","theme":"tt1-blocks","align":"full", "tagName":"header","className":"site-header"} /-->

In a recent Gutenberg PR WordPress/gutenberg#28088, we decided that the best path forward was to remove the static theme attribute provided by block templates theme files. The Gutenberg plugin now handles assignment of the correct theme attribute (see here and here) so that we can guarantee that it will match the current theme's stylesheet.

@carolinan
Copy link
Collaborator

I am not able to use the steps listed above for testing. I am not using wp-env. But as far as I can tell the pull request is correct.

The confusion lies in the description, the pr is very straight forward and all that was needed was to refer to WordPress/gutenberg#28088 🤷‍♀️

@jeyip
Copy link
Collaborator Author

jeyip commented Jan 20, 2021

The confusion lies in the description, the pr is very straight forward and all that was needed was to refer to WordPress/gutenberg#28088 🤷‍♀️

That's a wonderful suggestion! I removed the PR description since it seemed to be causing more confusion than helping and included a reference to #28088 instead. Thank you @carolinan 🙏

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @jeyip.

I just tested this PR against Gutenberg master and the templates + template parts appear correctly in the site editor, even when the theme is nested in a subdirecory.

However using the latest plugin release (9.8.0), the template parts do not resolve correctly.

Could we wait for WordPress/gutenberg#28088 to be included in a release before landing this PR, or is there a reason to ship this PR before then?

@jeyip
Copy link
Collaborator Author

jeyip commented Jan 21, 2021

Could we wait for WordPress/gutenberg#28088 to be included in a release before landing this PR, or is there a reason to ship this PR before then?

This makes sense to me 👍 We can revisit this once Gutenberg v9.9 is released

@jeyip
Copy link
Collaborator Author

jeyip commented Jan 27, 2021

Quick update:
The Gutenberg v9.9 release is being pushed back to align with the release of WordPress v5.7 beta 1. Both are slated for release on February 2nd.

Trying to solve merge conflict.
@carolinan
Copy link
Collaborator

I merged the changes for TT1 Blocks only.

@jeyip
Copy link
Collaborator Author

jeyip commented Feb 5, 2021

This PR originally updated twentynineteen-blocks, twentytwenty-blocks, and tt1-blocks. After giving it more thought, since Carolina already merged the changes for tt1-blocks, should we still go through the trouble of updating twentynineteen and twentytwenty block themes? I'm not sure if they're still being supported. @jffng

If not, I can close out this PR.

@jeyip
Copy link
Collaborator Author

jeyip commented Feb 19, 2021

Checking in again @jffng @kjellr. I'm leaning towards closing this PR, since it seems like twentynineteen and twentytwenty blocks aren't being actively supported as experiments anymore

@kjellr
Copy link
Collaborator

kjellr commented Feb 22, 2021

Yeah, I think it's fine to close, until we do a more thorough refresh of either of those two themes. Thanks, @jeyip!

@jeyip jeyip closed this Feb 22, 2021
@youknowriad youknowriad deleted the update/block-template-theme-attributes branch February 23, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
block-based theme A theme using HTML templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove theme attribute from template files
5 participants