search

WordPress / wporg-theme-directory

15 stars 6 forks source link

Add locale suggestion endpoint & block #52

Closed ryelle closed 3 months ago

ryelle commented 3 months ago

Fixes #27 — This adds two API endpoints, a general locale-banner and a version that accepts a theme slug (e.g., locale-banner/twentynineteen). I've mostly mirrored the plugins endpoint, though I broke the one function into a few steps.

On the homepage, the API will suggest other locales if the user has language hints in the request header. The API should never direct a person to the site they're currently on. On single themes, the API should suggest other rosetta sites only if the theme is translated into those languages.

I'm not sure if it would've been better to abstract out the plugin directory API code and make it reusable across both (I didn't want to get that in the weeds about it), but perhaps this can be a step towards that, if we want.

I also expect to reuse get_transalated_locales for #28.

Screenshots

Hacked a bit since these strings haven't been translated yet, but this is what the banner looks like. I can update this to match the outcome of https://github.com/WordPress/wordpress.org/issues/301.

Homepage Single pattern
Screen Shot 2024-05-07 at 17 38 22 Screen Shot 2024-05-07 at 17 38 16

I've tested a number of cases, here are a subset.

Case Result
Home/archives: English or rosetta site, no other languages requested Empty.
Home/archives: English site, Spanish requested & available The theme directory is also available in Español.
Home/archives: Rosetta site, site language (French) available, Spanish & Korean requested & available The theme directory is also available in 한국어 및 Español.
Single theme: English site, no other languages requested Empty.
Single theme: English site, Esperanto requested, unavailable This theme is not translated into Esperanto yet. Help translate it!
Single theme: English site, Spanish & Korean requested & available https://wordpress.org/themes/wp-json/wporg-themes/v1/locale-banner/twentytwentytwo/?new-theme=1' -H 'Accept-Language: en-US,en;q=0.8,ko-KR;q=0.5,es-ES;q=0.3 This theme is also available in 한국어 및 Español. Help improve the translation!
Single theme: Rosetta site, site language (Esperanto) unavailable This theme is not translated into Esperanto yet. Help translate it!
Single theme: Rosetta site, site language (Esperanto) unavailable, Spanish requested & available This theme is not translated into Esperanto yet. This theme is available in Español. Help improve the translation!
Single theme: Rosetta site, site language (Spanish) available, Korean requested & available This theme is also available in 한국어. Help improve the translation!

How to test the changes in this Pull Request:

Try out various combinations of header locale (Accept-Language header), theme, rosetta origin site, etc. This needs a sandbox to test for the locale database.

An example request, a single theme from the French rosetta site, with Spanish & Korean also suggested:

curl --socks5 localhost:8080 --resolve fr.wordpress.org:SANDBOX_IP 'https://fr.wordpress.org/themes/wp-json/wporg-themes/v1/locale-banner/twentytwentytwo/?new-theme=1' -H 'Accept-Language: en-US,en;q=0.8,ko-KR;q=0.5,es-ES;q=0.3'
StevenDufresne commented 3 months ago

I'm not sure if it would've been better to abstract out the plugin directory API code and make it reusable across both (I didn't want to get that in the weeds about it), but perhaps this can be a step towards that, if we want.

I think that rest-api-locale doesn't make sense to be shipped with the theme. At the very least, it probably makes more sense in the theme-directory plugin. I feel the same way for the rest-api itself. I wouldn't expect routes to be registered in the theme.

Apart from that code placement, I don't have a super strong opinion as to whether we need to go back and refactor the plugin directory code but it's obviously ideal to do so.

ryelle commented 3 months ago

I think that rest-api-locale doesn't make sense to be shipped with the theme. At the very least, it probably makes more sense in the theme-directory plugin. I feel the same way for the rest-api itself. I wouldn't expect routes to be registered in the theme.

I agree, generally, but here I was aiming for "moving fast", so I didn't want to create a chain of PRs and project dependencies, since the theme-directory plugin is still in meta.trac. I can move these routes over to the theme-directory plugin, though.

The other endpoints registered here are for favoriting, which are used in the favorite block - hence bundled with the theme for now. That too could possibly be extracted if we want to try a single favorite block for all the directories, but i wouldn't want to remove it unless the block moves too.

StevenDufresne commented 3 months ago

Yep understood. Can we move the locale endpoints over to the theme-directory plugin then and move forward with the rest?

ryelle commented 3 months ago

Closing this as https://github.com/WordPress/wporg-mu-plugins/pull/609 created the new API endpoint. The locale banner was added to the site in https://github.com/WordPress/wporg-theme-directory/commit/a4dc92d1f4fa188b17265bf636121e4ebb256151.