-
-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Localize keyboards share and install pages 🗺️ #623
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
User Test ResultsTest specification and instructions User tests are not required |
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.
keyboards/install.php
Outdated
| $downloadKeymanTitle = _m("download_keyman_title"); | ||
| $withPlayStore = _m("with_play_store", $h['name']); | ||
| $installFromPlayStore = _m("install_from_play_store"); | ||
| $installKeymanAndKeyboard = _m("keyman_and_keyboard_for_platform", $h['name'], "Android"); | ||
| $alreadyInstalled = _m("already_installed"); | ||
| $downloadJustKeyboard = _m("download_just_keyboard"); | ||
| $downloadKeymanUrl = PlayStore::url . "&referrer=" . rawurlencode($referrer); | ||
| $andThenInstallInApp = _m("and_then_install_in_the_app"); |
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 not a big fan of introducing lots of variables which results in repetition and divergence of variable names (e.g. $andThenInstallInApp vs and_then_install_in_the_app). Is there a way we just embed in the strings instead?
e.g. in Locale.php, add the following line to declare a global variable $_m:
$_m = '_m';echo <<<END
... {$_m('already_installed')} ...This ends up calling _m(...) which is kinda weird and kinda helpful.
keyboards/install.php
Outdated
| use Keyman\Site\com\keyman\Util; | ||
| use Keyman\Site\com\keyman\Locale; | ||
|
|
||
| define('LOCALE_KEYBOARD_INSTALL', 'keyboards/install'); |
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.
Should this be LOCALE_KEYBOARDS_INSTALL for consistency?
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 suppose? In keyboard-details, we used LOCAL_KEYBOARD_DETAILS
keyman.com/_includes/includes/ui/keyboard-details.php
Lines 14 to 17 in b161eb8
| define('LOCALE_KEYBOARD_DETAILS', 'keyboards/details'); | |
| $_m_KeyboardDetails = function($id, ...$args) { | |
| return Locale::m(LOCALE_KEYBOARD_DETAILS, $id, ...$args); | |
| }; |
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.
Yeah, I saw that this morning and then when I saw it here also I realized that we should probably be consistent and it's worth fixing up before we trip over ourselves.
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.
Updated the 3 pages to use LOCALE_KEYBOARDS_
keyboards/share.php
Outdated
| define('LOCALE_KEYBOARD_SHARE', 'keyboards/share'); | ||
| $_m_KeyboardShare = function($id, ...$args) { | ||
| return Locale::m(LOCALE_KEYBOARD_SHARE, $id, ...$args); | ||
| }; | ||
| function _m_KeyboardShare($id, ...$args) { | ||
| return Locale::m(LOCALE_KEYBOARD_SHARE, $id, ...$args); | ||
| } |
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.
Dittos
Co-authored-by: Marc Durdin <marc@durdin.net>
mcdurdin
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.
LGTM!
Follows #622 for #384
This localizes the keyboards share and install pages
.htaccess also updated so the query string (lang=) gets appended on the share page
Test-bot: skip