Details
- Reviewers
mjf dbaker flod - Group Reviewers
Restricted Project - Commits
- rMOZILLACENTRAL695e7bce3a5f: Bug 1818612 - reduce SDP history real estate in about:webrtc;r=mjf,fluent…
- Bugzilla Bug ID
- 1818612
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Lint Not Applicable - Unit
Tests Not Applicable - Build Status
Buildable 513007 Build 609476: arc lint + arc unit
Event Timeline
Sorry, but we shouldn't ask ~100 locales to retranslate all the strings for sake of having consistent IDs.
The other issue is generating IDs at run-time. This is not considered best practice, because (among other things) it creates maintainability issues: you won't find the string ID anywhere in the code, and if you need to change the string in the future you'd still need to add code to accommodate for that.
You can still generate an internal ID, but then have a map to connect that internal ID to a fully formed Fluent message ID.
| toolkit/locales/en-US/toolkit/about/aboutWebrtc.ftl | ||
|---|---|---|
| 271 | Fluent supports group level comments (with two hashes). This comment should apply to both strings, not just the one following. https://projectfluent.org/fluent/guide/comments.html Not strictly relevant for localization, but I don't think it's common practice to have lowercase labels in buttons? | |
| toolkit/locales/en-US/toolkit/about/aboutWebrtc.ftl | ||
|---|---|---|
| 271 | Thank you, and thanks for the link! | |
Code analysis found 1 defect in the diff 687305:
- 1 defect found by fluent-lint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 687305.
1 issue closed compared to the previous diff 687305.
If you see a problem in this automated review, please report it here.
I like how this looks and it makes looking at the remote/local sdp more pleasant. Nice!
@flod, I believe this addresses your concerns. If I were to fix the capitalization of the other "hide" "show" strings, will that likewise cause similar amounts of localization churn? Would it make any difference in induced labor if I were to correct all of those in their own patch? Would it be preferable for me to do that here?
| toolkit/content/aboutwebrtc/aboutWebrtc.css | ||
|---|---|---|
| 169 | This yellow makes the white text difficult to read when using dark mode. Maybe a different color or having the text be a dark color could help. | |
Thanks, this looks good.
If I were to fix the capitalization of the other "hide" "show" strings, will that likewise cause similar amounts of localization churn?
I missed there were already other lowercase button labels.
https://searchfox.org/mozilla-central/search?q=-show-&path=aboutWebrtc.ftl&case=false®exp=false
Normally capitalization changes don't require new IDs and retranslation, but in this case we would need to signal locales to avoid inconsistencies, and that can only be done by changing the IDs. At this point, I would use lowercase for these new labels as well (like you did initially), and keep the page consistent.
Would it make any difference in induced labor if I were to correct all of those in their own patch? Would it be preferable for me to do that here?
I assume with "all of those" you mean the English labels, not the localized ones? If we want to fix them, there's no way to avoid churn.
And now I just saw the email for D171306, which introduces 2 other strings that could be capitalized, making it potentially 5 uppercase (Show tab is already inconsistent), and 3 lowercase.
At this point, let's keep this patch as is, and fix the remaining 3 labels with a new IDs. Feel free to tag me for reviewing that patch.