-
Notifications
You must be signed in to change notification settings - Fork 389
[personal-wp] Add app catalog #3173
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
Conversation
7cdbb30 to
c1b2abd
Compare
055eea1 to
6aff978
Compare
c1b2abd to
c94e2d5
Compare
6aff978 to
f651ba1
Compare
| * This map provides explicit mappings for cases where the conversion | ||
| * isn't straightforward (e.g., "de" -> "de_DE", not "de"). | ||
| */ | ||
| const BROWSER_TO_WP_LOCALE: Record<string, string> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! What's the source of that? If we're confident every single item of this list checks out, let's put it somewhere in core (could also be after this PR). We always struggle with the right locale strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually https://github.com/GlotPress/GlotPress/blob/develop/locales/locales.php. I created a script to get the data from there and convert it to a json.
| @$relay_http_code_and_initial_headers_if_not_already_sent(); | ||
| } | ||
| // Close cURL session | ||
| curl_close($ch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's that about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the cherry-pick from #3164 (comment) that I mentioned in the PR description. This is just to make it work for testing locally with a PHP 8.5, I can remove it and we can discuss more over there.
| return cachedResponse; | ||
|
|
||
| // The Cache API only supports GET requests | ||
| const canCache = request.method === 'GET'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Was this method ever called for non-GET requests? If yes, that sounds like an issue in itself. I'd rather throw an error here in this code path than silently accept a wrong input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, cherry picked from #3164, we can discuss there and I'll remove it from this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small but important nitpicks
978b7de to
ee9ad88
Compare
e696f40 to
3318673
Compare
3318673 to
c9943c8
Compare
ee9ad88 to
48a1290
Compare
7c6e8a0 to
5e694fc
Compare
48a1290 to
55a7f29
Compare
36800a5 to
b17472b
Compare
| * 4. Outputs JSON to src/lib/personalwp/locale-map.json | ||
| */ | ||
|
|
||
| import * as fs from 'fs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking: This script seems really fragile. Could GlotPress just load that data from a json file and then we'd reuse that json file here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check with @amieiro: Are there long term plans like that for GlotPress?
| if (url.startsWith('data:application/json;base64,')) { | ||
| try { | ||
| const base64 = url.replace('data:application/json;base64,', ''); | ||
| const json = decodeURIComponent(escape(atob(base64))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atob has no UTF-8 support:
php > echo base64_encode('łącznik');
xYLEhWN6bmlr
console.log(atob('xYLEhWN6bmlr'))
VM720:1 ��cznik
Also, btoa would throw on the opposite operation.
Let's use the decodeBase64ToString and encodeStringAsBase64 helpers for working with base64.
| return url; | ||
| } | ||
|
|
||
| function looksLikeBlueprint(text: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not terrible but again, I think it's duplicating an existing helper from the website package. We can move forward here but I'm just marking it for some refactoring later on to extract shared dependencies.
| const urlPath = new URL(trimmed).pathname; | ||
| const filename = urlPath.split('/').pop() || ''; | ||
| if (filename) { | ||
| title = filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably tweak this quite a bit to account for different extensions, word separators, and totally messy paths that don't contain any useful information. Not a blocker at all.
| * @param browserLang - Browser language code (e.g., "en-US", "de", "pt-BR") | ||
| * @returns WordPress locale (e.g., "en_US", "de_DE", "pt_BR") or null if no mapping | ||
| */ | ||
| export async function browserLanguageToWpLocale( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm nervous about this. Can we have some test coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add more tests.
packages/playground/personal-wp/src/components/menu-overlay/index.tsx
Outdated
Show resolved
Hide resolved
adamziel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this, but I also don't see any risks to playground.wordpress.net and I don't see any blockers in the code.
55a7f29 to
45accb9
Compare
Shows a checkmark icon for 2 seconds after copying to indicate success.
b17472b to
81095b9
Compare
Motivation for the change, related issues
Adds an "Install Apps" section to the Personal Playground menu overlay. Users can browse available apps from the WordPress blueprints repository and install them with a single click.
Also includes automatic language detection that configures WordPress based on the user's browser language.
Finally, this also adds the ability to add custom apps by just pasting a blueprint while on the apps screen. This is to make it easier to develop new apps and conveniently reinstall them via a click.
Note: When WordPress/blueprints#166 is merged we need to update the
my-wordpressbranch in the URLs.Screenshot
Implementation details
App catalog (
menu-overlay/index.tsx):https://raw.githubusercontent.com/WordPress/blueprints/my-wordpress/apps.jsoni18n support (
i18n.ts):setSiteLanguageblueprint step prepended to the personal blueprintTesting Instructions (or ideally a Blueprint)
npm run devAdditionally: paste a blueprint while on the apps page to test the custom apps.