Move Woo Blocks template directories to latest Gutenberg convention #5464
Conversation
sunyatasattva
added
focus: FSE
|
Size Change: 0 B Total Size: 819 kB ℹ️ View Unchanged
|
sunyatasattva
force-pushed
the
fix/5343-change-block-templates-directories
branch
2 times, most recently
from
0916a21 to
4abced5
Compare
sunyatasattva
force-pushed
the
fix/5343-change-block-templates-directories
branch
from
4abced5 to
e0e4443
Compare
sunyatasattva
force-pushed
the
fix/5343-change-block-templates-directories
branch
from
e0e4443 to
8db7484
Compare
There was a problem hiding this comment.
This is testing well and code looks good, thanks for working on this @sunyatasattva!
Heads-up that we will need to update the path in this line:
Is that something we can do as part of this PR?
Besides that, everything looks good to me, so pre-approving.
| public static function generate_template_slug_from_path( $path ) { | ||
| $template_extension = '.html'; | ||
|
|
||
| return basename( $path, $template_extension ); |
There was a problem hiding this comment.
Thanks! It was also necessary, by the way. I didn't simplify it just for the sake of it. But because our dir structure became templates/templates, the original substr code would not work anymore.
@Aljullu I updated that code, but I'm not 100% sure how to test it. Would you mind testing the mini cart part and perhaps add testing instructions to the OP for when the release is made? I also snuck in the change of order between child and parent theme priority. Lastly, I did a search on It mentions that it looks for the Would this mean we might have to change those somewhere? |
There was a problem hiding this comment.
@Aljullu I updated that code, but I'm not 100% sure how to test it. Would you mind testing the mini cart part and perhaps add testing instructions to the OP for when the release is made?
Done! 🙂 Right now, it could be reproduced with Gutenberg disabled, but once #5448 is merged, I think it will require testing with WP 5.8 and Gutenberg disabled.
I also snuck in the change of order between child and parent theme priority.
Thanks! That still loads the "Parent theme templates folder" before "Child theme block-templates folder", if I'm reading it correctly. I don't have a strong opinion on this one, but I feel it should be the other way around (child theme wins even if it's using the old directory name). In any case, not blocking this PR.
Lastly, I did a search on
block-templateand I found this function:
Good catch! I think that comment is outdated and needs to be updated.
It mentions that it looks for the
block-templatesdirectory, but I'm not sure where it does that. Perhaps one of the functions:gutenberg_get_block_templateorget_block_templatewhich I'm not sure where they come from.
I think that happens when get_single_block_template is hooked into get_block_file_template. If you keep looking, get_single_block_template() calls get_block_templates() which, in its turn, calls get_block_templates_from_woocommerce(), where templates are loaded from files, if I'm not wrong.
gutenberg_get_block_template() calls get_block_template() are Gutenberg and WordPress globals, but I don't think we need to modify them here.
🤔 I see what you mean. You are probably right, that should likely be the order… though possibly unlikely that a parent theme has the new convention and the child the old one. I mean, that's possible, but I get the feeling that this is a relatively edge case. Should I create a follow-up issue so we don't forget?
Updated the comment!
Thanks a lot for taking the time to clarify! |
I wonder if we could do here the same we did in WC core. This way, we can easily change the order templates are loaded. |
|
|
||
| /** | ||
| * Constructor. | ||
| */ | ||
| public function __construct() { | ||
| // This feature is gated for WooCommerce versions 6.0.0 and above. | ||
| if ( defined( 'WC_VERSION' ) && version_compare( WC_VERSION, '6.0.0', '>=' ) ) { | ||
| $this->templates_directory = plugin_dir_path( __DIR__ ) . 'templates/' . self::TEMPLATES_DIR_NAME; | ||
| $this->template_parts_directory = plugin_dir_path( __DIR__ ) . 'templates/' . self::TEMPLATE_PARTS_DIR_NAME; | ||
| $root_path = plugin_dir_path( __DIR__ ) . self::TEMPLATES_ROOT_DIR . DIRECTORY_SEPARATOR; |
There was a problem hiding this comment.
Can we not replace self::TEMPLATES_ROOT_DIR with BlockTemplateUtils::DIRECTORY_NAMES['TEMPLATES'] ?
There was a problem hiding this comment.
Technically, we can because they have the same value. However, the reason why I added TEMPLATES_ROOT_DIR in the first place is because they represent two different things:
BlockTemplateUtils::DIRECTORY_NAMES['TEMPLATES']represents theblock-templatesdirectory name in themes.BlockTemplatesController::TEMPLATES_ROOT_DIRrepresents the root directory of all templates in our own repo. For example, we have email templates.
The just happen to be both named templates now, so we have the awkward path templates/templates (which makes me sad :'(). But technically they are separate entities that could also be changed independently at any point.
Does that make any sense?
Thought that would be the case, makes sense. Thanks! |
With WordPress/gutenberg#36647, Gutenberg 12.1.0 has changed the convention for the directory paths from
block-templatesandblock-template-partstotemplatesandpartsrespectively.We are also moving our repo to that convention.
Fixes #5343
Note
Tagging also @tjcafferkey as a reviewer, just to page him (as he had requested this PR to be split in two); however, he's going to be AFK for the next week, so it's really just a CC.
Testing
Manual Testing
How to test the changes in this Pull Request:
User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
Changelog