Bug 1187668 - update settings l10n api uses #31107
Conversation
|
Autolander could not find a bug number in your pull request title. All pull requests should be in the format of: Bug [number] - [description]. |
zbraniecki
changed the title
zbraniecki
changed the title
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
92471a2
to
3b31a39
Compare
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
3b31a39
to
d2c170d
Compare
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
d2c170d
to
223ecbc
Compare
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
223ecbc
to
f9887d1
Compare
| @@ -29,14 +29,14 @@ <h2 data-l10n-id="fxa-confirm-account">Confirm Your Account</h2> | |||
| </header> | |||
| <ul> | |||
| <li class="description"> | |||
| <p data-l10n-id="fxa-verification-email-sent-msg" id="fxa-unverified-text"> | |||
There was a problem hiding this comment.
does it intended to remove the l10n here?
There was a problem hiding this comment.
yes. The entity requires an attribute (email address), so it will not work without it, so there's no reason to keep it in HTML since it has to be defined from JS as l10n-id/l10n-args couple and it is in apps/settings/js/firefox_accounts/panel.js#showUnverifiedPanel
|
treeherder fail, please fix the tests |
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
f9887d1
to
7a2f211
Compare
|
it was just a treeherder failure over the weekend. The tests are green now (except of some unrelated privacy-panel bug that is on all treeherder runs) |
| } else if (errorMessage === 'Authentication Failed') { | ||
| msg = msg + '\n' + _('error-pair-pincode'); | ||
| l10nId = 'error-pair-pincode'; |
There was a problem hiding this comment.
The logic here is changed. It was an error "title + body", but it now only remains body.
There was a problem hiding this comment.
So I think you need to update string as you did for unpair-confirm. It's easier. :)
There was a problem hiding this comment.
The reason why I decided to not modify the ID was because previously it would build message like this:
Unable to pair devices\nUnable to pair with the device. Check that the PIN is correct.
which I changed to either first line or the latter. Since all messages have the same "Unable to pair with the device" I don't feel like removing the first line changes anything.
Can you look at settings.en-US.properties and let me know what you think?
If you'll still believe I should, I'll change the IDs :)
There was a problem hiding this comment.
Yep, I noticed there are repeated words but it was UX design of "title" and "body", so we can't remove one of them, otherwise the UI will be a bit weird.
There was a problem hiding this comment.
Hmm, I actually think that the UI would be less weird :)
Would you be ok if I took some screenshots with this change (and without) and NI'ed UX in the bug?
There was a problem hiding this comment.
ok, I don't even know how to get this code to run :(
The only code piece related to the string error-pair-title I can run is using this code: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/bluetooth/panel.js#L593-L599 and it's already properly separating the entities.
The only other place where this l10nId is used is here and I don't know how to trigger this code path.
Can you help me? Can we move it to use DialogService.alert?
There was a problem hiding this comment.
Oh! You remind me that here is a legacy code for Bluetooth API version 1. Sorry I didn't think of it. I asked Ian who wrote the newer version (bluetooth/panel.js#L593-L599) and he said UX reviewed that part. So feel free to ignore my previous comments.
| year: 'numeric', | ||
| month: 'long', | ||
| day: '2-digit', | ||
| }); |
There was a problem hiding this comment.
It's great to see we use native API of Date object instead of DIY!
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
7a2f211
to
bb96f35
Compare
| @@ -303,7 +303,7 @@ message-read-reports-details = Request read report for each message sent | |||
| # force the proper directionality of parentheses at the end of the string | |||
| # regardless of the direction of the surrounding layout. Remove them only | |||
| # if the translation puts them between other text. | |||
| mmsTitle.innerHTML= ‪Multimedia Messaging (MMS)‬ | |||
| mmsTitle= ‪Multimedia Messaging (MMS)‬ | |||
There was a problem hiding this comment.
@stasm - in bug 1139031 you recommended this solution. If I'm correct that is either not needed (because we do inject it in l10n.js for fallback languages) or it should be affecting all <h2> elements and should use CSS dir:auto instead.
Am I correct? I'd like to remove those characters from localization files
There was a problem hiding this comment.
AFAIR, this was an edge case in which the translation of this string was missing in Arabic and on buildtime we would fall back to English. The parenthesis would take the directionality of the surrounding Arabic text.
Now that I think about this, it almost seems like the best solution would be to have L20n's HTML bindings do something like this:
const sheet = doc.head.appendChild(document.createElement('style')).sheet;
sheet.insertRule('[data-l10n-id] { unicode-bidi: isolate; }', 0);There was a problem hiding this comment.
so, I tried setting unicode-bidi: isolate on <h2> it and it didn't work for some reason. I decided to instead of that use <bdi> tag inside <h2> and that works well.
| @@ -150,7 +154,8 @@ define(function(require) { | |||
| */ | |||
| _checkForUpdates: function uc__checkForUpdates() { | |||
| if (!navigator.onLine) { | |||
| alert(this._('no-network-when-update')); | |||
| navigator.mozL10n.formatValue('no-network-when-update').then( | |||
| msg => alert(msg)); | |||
There was a problem hiding this comment.
so, trying to see what will happen if formatValue takes time, I delayed it here for 5s.
The result was that the UI was a bit confusing since nothing has happened.
I can add disabled state on the button and remove it when formatValue resolves. It could be also a good idea to add it for when the update is being performed and remove when the checking is complete.
Would that work for you @evelynhung ?
There was a problem hiding this comment.
Yep, it's a good idea to disable the button before formatValue resolves, I assume usually users won't notice this disabled state though. Would it be that much a 5s delay?
There was a problem hiding this comment.
absolutely not. Actually, at this moment there is no way for it to take any time because all this code will be fired after we already translated DOM and displayed the UI (which means that we have all resources in memory).
In the future we would like to consider switching to runtime language fallbacking which means that in error scenarios (when for some reason the string cannot be retrieved/resolved in user selected language) we will load fallback locale and use the string from there, this can take ~100-200ms.
But I don't ever expect a scenario when retrieving a string takes longer than than, and in all non-error scenarios it is literally below 5ms (timeout of a Promise).
So, while I like to think of formatValue as "it should work even if it would take 5s", I don't believe it's a realistic worry.
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
505ed23
to
ba95822
Compare
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
ba95822
to
a558fb7
Compare
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
a558fb7
to
aa2cb1c
Compare
| # | ||
| # RTL locales should have a translation similar to this: | ||
| # hotspot-wpa-psk.innerHTML= ‪WPA (TKIP)‬ | ||
| # hotspot-wpa2-psk.innerHTML= ‪WPA2 (AES)‬ |
There was a problem hiding this comment.
Why are you removing this comment? Most likely, RTL languages won't translate these acronyms, and without the unicode marks, the parens will break. Should we add unicode-bidi: isolate to CSS to prevent this?
There was a problem hiding this comment.
So, once again, I tried unicode-bidi:isolate for mmsTitle and it did not fix it there. Can I just put it in <bdi>?
There was a problem hiding this comment.
Yeah, I don't get it: http://jsfiddle.net/7qopv17s/
There was a problem hiding this comment.
I readded the comment. unicode-bidi: isolate doesn't seem to work at all on anything, and also putting <bdi> inside the <option> doesn't seem to work so I for this case we're stuck with ‪/c.
zbraniecki
force-pushed
the
1187668-update-settings-l10n-api-uses
branch
from
aa2cb1c
to
a39c7a5
Compare
…n-api-uses Bug 1187668 - update settings l10n api uses. r=evelyn
https://bugzilla.mozilla.org/show_bug.cgi?id=1187668