Closed Bug 1105648 Opened 5 years ago Closed 5 years ago

Refactor SystemBanner's L10n API use

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gsvelto, Assigned: zbraniecki)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

PR
46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
ferjm
: review+
Details | Review
L10n strings shouldn't contain formatting characters such as '\n'. One is present in the `low-device-storage' string, see here:

https://github.com/mozilla-b2g/gaia/blob/85739c1733d7437b880084e50153f4c788812cdf/apps/system/locales/system.en-US.properties#L491
Blocks: 861921
Attached file PR
Assignee: nobody → anygregor
Whiteboard: [systemsfe]
Attachment #8537467 - Flags: review?(ferjmoreno)
Comment on attachment 8537467 [details] [review]
PR

Thank you Gregor!

I haven't tried the patch but I guess that removing the "\n" from that string will make the notification to look like: "Device storage low. Less than..." all in the same line, while UX defined it to be in two lines. Check the screenshot at [1].

What I would do is to get rid of the low-device-storage string, add the "Device storage low." phrase to free-space [2] and unknown-free-space [3] strings and then do something like:

[...]
free-space.innerHTML[one]=<span>Device storage low.</span><span>Less than {{value}} {{unit}} remaining.</span>
[...]

I a pretty bad at l10n stuff so there might be a better way. What do you think?

[1] https://bug861921.bugzilla.mozilla.org/attachment.cgi?id=746407
[2] https://mxr.mozilla.org/gaia/source/apps/system/locales/system.en-US.properties#495 
[3] https://mxr.mozilla.org/gaia/source/apps/system/locales/system.en-US.properties#502
Attachment #8537467 - Flags: review?(ferjmoreno)
Issue with the current PR: you need a new string ID, otherwise nobody will know that they need to remove the stray \n

I would honestly avoid using innerHTML to display a message on two lines (especially considering that we're trying to move away from it). Can't we just use 2 separate strings and manage the markup in the code?
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → All
Using .innerHTML is not that bad. The problem is that it will not work in this case, because SystemBanner API doesn't use declarative L10n approach and does not set the L10nId/L10nArgs on the Node. Instead it takes the resolved string and assign it to textContent of the node. This will not work well.

What I'm thinking about is that it seems that would make sense to refactor SystemBanner API to follow [1] and then it should be easy to decide if we want to extend the API to support two-liners or use entity.innerHTML.

(I'd vote for extending the API to support two liners :))

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

Ferjm: If you're ok with this, I'd be happy to take this bug and use it to refactor SystemBanner and its 5 uses.
Oh, that's actually a question to gwagner.
Flags: needinfo?(anygregor)
Attached file pull request
This is just a proof of concept. I didn't fix tests yet.

Let me know what you think :)
Comment on attachment 8538062 [details] [review]
pull request

Fernando, would you be willing to look at this patch?
Attachment #8538062 - Flags: review?(ferjmoreno)
Duplicate of this bug: 1018346
See Also: → 1163582
Summary: The `low-device-storage' l10n string ends with a '\n' character → Refactor SystemBanner's L10n API use
Assignee: anygregor → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8538062 [details] [review]
pull request

LGTM. Thanks!
Flags: needinfo?(anygregor)
Attachment #8538062 - Flags: review?(ferjmoreno) → review+
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #3)
> Issue with the current PR: you need a new string ID, otherwise nobody will
> know that they need to remove the stray \n

Wow… :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Stef, sorry, but I believe that your comment is useless as it doesn't explain what you mean. Also, you should not modify the state of the bug if you're not an owner.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> Stef, sorry, but I believe that your comment is useless as it doesn't
> explain what you mean. Also, you should not modify the state of the bug if
> you're not an owner.

OK, I'm sorry, I didn't meant to be pointlessly disruptive and I won't modify the state of the bugs I'm not assigned to.
I chatted with Stef on IRC. He referred to comment 3 where Flod pointed out that the patch removed trailing "\n".

We tested it and came to the conclusion that the trailing "\n" does not affect the UI because HTML removes it. So we can safely keep the entity ID.
You need to log in before you can comment on or make changes to this bug.