Skip to content
This repository was archived by the owner on Feb 17, 2025. It is now read-only.

Conversation

arthur791004
Copy link
Contributor

Changes proposed in this Pull Request:

  • Add the load_theme_textdomain function call to load the translation so that we could get the translated theme patterns 🙂

Related issue(s): 118-gh-Automattic/ganon-issues

@pbking
Copy link
Contributor

pbking commented Sep 1, 2022

I think that any of the Blockbase children could be handled by Blockbase itself. This seemed to work from there to load translations for anything Blockbase.

		// Make theme patterns available for translation.
		load_theme_textdomain( wp_get_theme()->get( 'TextDomain' ) );

But during testing I noticed that the localized resources that were from Blockbase weren't getting provided, only those from the child (Zoologist, etc). This might be related to how the patterns are created (via code rather than via a template as most other themes do). Switching the 404.php to a template seemed to get the expected value rendered.

@arthur791004
Copy link
Contributor Author

Sorry for the late reply. I'll check your way when I have the time! Looks like it's possible to add the function call in single place 👍

@pbking
Copy link
Contributor

pbking commented Sep 9, 2022

I did some tinkering and (I'm not convinced this is the way to do it but) I found that for Blockbase CHILD themes any pattern that comes from the PARENT (blockbase) isn't translated unless the function is called for BOTH domains:

		load_theme_textdomain( 'blockbase' );
		if ( ! "blockbase" === wp_get_theme()->get( 'TextDomain' ) ) {
			load_theme_textdomain( wp_get_theme()->get( 'TextDomain' ) );
		}

I've been doing my testing with Geologist mostly.

@arthur791004
Copy link
Contributor Author

Nice, I'll have a try! thanks 👍

@arthur791004
Copy link
Contributor Author

I've added the function call to the "blockbase" theme so that the patterns from the "blockbase" will be translated. It looks like lots of themes are still not the child theme of the blockbase, so my opinion is to add the function call to every theme and then we won't forget to call it for the newly created theme. What do you think?

@pbking pbking force-pushed the feat/load-theme-translations branch from e2085c4 to c58bb66 Compare October 17, 2022 15:09
@pbking
Copy link
Contributor

pbking commented Oct 17, 2022

I started with a rebase to bring everything current.

I've updated the logic in Blockbase to handle the child themes so they don't need to include that logic.

I've also removed the location from the calls since the value that was being passed was the default; no need to define it and it simplifies the method call.

I've added the call to a few additional themes, including Block Canvas so any future themes based on that should include that logic.

I believe that this is ready for review and hopefully we can get it shipped.

@pbking pbking requested a review from a team October 17, 2022 15:14
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

It looks like you've covered all Blockbase + children, and all our current block themes 👍

I don't seem to get the same results locally and on dotcom.

This is what I'm seeing on Skatepark's 404 on wpcom:

Before After
image image

But this is what I'm seeing on my local site:

image

I tested in Swedish, with the same translation files in both. I copied the contents of the languages folder over from the theme on wpcom to make sure I definitely had the same translations.

I'm confident this is working on wpcom, so I'm happy for this to be merged for this use case.

I may be testing this incorrectly locally, so I'd appreciate either some help testing locally (or someone else to test!) or perhaps we should look into a local fix next.

@arthur791004
Copy link
Contributor Author

Thanks for testing! I believe this is good to ship 🙂

But I'm not familiar with the flow of deploying the PR in this repo, could you help to do it? thanks!

@pbking
Copy link
Contributor

pbking commented Oct 28, 2022

I tinkered around with this as much as I could to see why things weren't getting translated in wporg-land and came up empty.

I'm keen to get this into wpcom, and there's no negative side to it existing as far as I can tell so I'm going to go ahead and bring this in and get it shipped. But I agree that we need to look into:

  • Why these patterns aren't getting translated WITHOUT this call
  • Why these patterns aren't getting translated outside of wpcom (with or without this call)

@pbking pbking merged commit 721d8e8 into trunk Oct 28, 2022
@pbking pbking deleted the feat/load-theme-translations branch October 28, 2022 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants