Update locale forwarding via path #61
Conversation
|
cc @WordPress/openverse-developers |
|
Before merging this it needs the new functionality to be put behind a theme setting. |
sarayourfriend
force-pushed
the
update/openverse-locale-forwarding
branch
from
dd98c6d to
fc433fc
Compare
|
Okay this is ready for review now. @WordPress/openverse-developers please take a look at this, thanks! |
| @@ -5,7 +5,7 @@ document.addEventListener('DOMContentLoaded', () => { | |||
| .toLocaleLowerCase() | |||
| .replace(openverseSubpath, '') | |||
| .replace(/^\/$/, ''); // Remove Openverse site subpath | |||
| iframePath = `${openverseUrl}${iframePath}${query}`; // Add domain and query | |||
| iframePath = `${openverseUrl}${localeSlug ? `/${localeSlug}` : ''}${iframePath}${query}`; // Add domain and query | |||
There was a problem hiding this comment.
| iframePath = `${openverseUrl}${localeSlug ? `/${localeSlug}` : ''}${iframePath}${query}`; // Add domain and query | |
| iframePath = `${openverseUrl}${localeSlug ? `${localeSlug}` : ''}${iframePath}${query}`; // Add domain and query |
There was a problem hiding this comment.
This slash causes the openverse_embed url have 2 slashes: <iframe id="openverse_embed" src="http://localhost:8443//ru">.
Without it, everything works, and if I use the branch from the translation banner fix PR, I can see the banner!
There was a problem hiding this comment.
Huh, I don't see that locally. For example, if I change the get_locale call to just be pt_BR I see this logged:
Navigating iframe to https://search-staging.openverse.engineering/pt-br
There was a problem hiding this comment.
Ah, I think this depends on what you set the ov_embed_url value to in customizer. If you add a trailing slash to that then it doubles the slash.
The default value does not include a trailing slash and I've just been testing by changing the subdomain to search-staging so that's what I haven't seen the double slash locally.
I guess we could make the code detect whether a the trailing slash is there and add it conditionally.
There was a problem hiding this comment.
The way I was testing it is by setting the language of the site in wp-admin. So, the top bar would display in the language I selected.
| @@ -5,7 +5,12 @@ document.addEventListener('DOMContentLoaded', () => { | |||
| .toLocaleLowerCase() | |||
| .replace(openverseSubpath, '') | |||
| .replace(/^\/$/, ''); // Remove Openverse site subpath | |||
| iframePath = `${openverseUrl}${localeSlug ? `/${localeSlug}` : ''}${iframePath}${query}`; // Add domain and query | |||
| const localePart = localeSlug | |||
| ? openverseUrl.endsWith('/') | |||
|
We can also remove step 3 from the README instructions, as the mu-plugin is already set up, at least it is for me? |
That's just you 🙂 On a fresh clone they're set up by |
https://meta.trac.wordpress.org/ticket/6105
Forwards the locale via the iframe's path rather than relying on a cookie.
To test, follow setup instructions in the README of the wporg-openverse theme. Set the "Use path based locale forwarding" setting to on by checking the box in the customizer. And redirect the iframe to use staging or localhost as production does not yet support path based locales.
Next, to test that the locales are working, replace the call to
get_locale()with awp_localestring (likept_BRetc) to verify that it is working to forward the locale as expected (as far as I know there's no way to test this "for real" locally as subdomain locale choosing isn't supported in the local wp-env when I tried it).Be sure to navigate around the various pages of the application. Pay special attention to the URL being shown in the host frame. It should never include the locale slug.
Next, test it with the
en_USlocale and make sure everything works, again paying attention to the locale slug.Finally, turn off the path based locale forwarding setting and switch the frame back to production. Ensure that it continues to work as expected, especially the host frame URL updates. I'm not sure how to test other locales other than again, manually replacing the call to
get_locale()with a locale string of choice. I noticed that it takes at least one page navigation for the locale to actually propagate via the cookie. This is the same as the behavior ontrunk.