RESOLVED FIXED 168465
Web Inspector: Loc: Zoom level in Settings tab needs to use localized formatting
https://bugs.webkit.org/show_bug.cgi?id=168465
Summary Web Inspector: Loc: Zoom level in Settings tab needs to use localized formatting
Blaze Burg
Reported 2017-02-16 14:16:35 PST
Intl to the rescue!
Attachments
[AFTER] tr-TR locale (125.46 KB, image/png)
2017-02-16 14:17 PST, Blaze Burg
no flags
Patch (2.00 KB, patch)
2017-02-16 14:22 PST, Blaze Burg
no flags
Patch v2 (1.75 KB, patch)
2017-02-16 15:10 PST, Blaze Burg
no flags
Blaze Burg
Comment 1 2017-02-16 14:17:32 PST
STEPS TO REPRODUCE: - Open Settings Tab - Look at Zoom Level: section EXPECTED: With en-US locale, should be 100%, 120%, etc. With tr-TR locale, should be %100, %120, etc.
Blaze Burg
Comment 2 2017-02-16 14:17:51 PST
Created attachment 301823 [details] [AFTER] tr-TR locale
Blaze Burg
Comment 3 2017-02-16 14:22:47 PST
Joseph Pecoraro
Comment 4 2017-02-16 14:30:06 PST
Comment on attachment 301824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301824&action=review > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:188 > - option.textContent = `${Math.round(level * 100)}%`; > + option.textContent = formatter.format(level); We have Number.percentageString in Utilities.js. Should we just use that?
Blaze Burg
Comment 5 2017-02-16 15:08:28 PST
(In reply to comment #4) > Comment on attachment 301824 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301824&action=review > > > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:188 > > - option.textContent = `${Math.round(level * 100)}%`; > > + option.textContent = formatter.format(level); > > We have Number.percentageString in Utilities.js. Should we just use that? Had no idea. Yes, it works the same it seems.
Blaze Burg
Comment 6 2017-02-16 15:10:43 PST
Created attachment 301836 [details] Patch v2
Matt Baker
Comment 7 2017-02-17 09:55:02 PST
Comment on attachment 301836 [details] Patch v2 Text isn't being localized with the patch applied
Blaze Burg
Comment 8 2017-02-17 14:00:57 PST
(In reply to comment #1) > STEPS TO REPRODUCE: > - Open Settings Tab > - Look at Zoom Level: section > > EXPECTED: > > With en-US locale, should be 100%, 120%, etc. > With tr-TR locale, should be %100, %120, etc. To get tr-TR locale on macOS, go to System Preferences > Language & Region and add Turkish/Türkçe, then drag it to the top position. Relaunch Safari.
Blaze Burg
Comment 9 2017-02-17 14:01:13 PST
(In reply to comment #7) > Comment on attachment 301836 [details] > Patch v2 > > Text isn't being localized with the patch applied It works for me. Did you change the system language?
Matt Baker
Comment 10 2017-02-17 14:15:47 PST
(In reply to comment #9) > (In reply to comment #7) > > Comment on attachment 301836 [details] > > Patch v2 > > > > Text isn't being localized with the patch applied > > It works for me. Did you change the system language? Works! My mistake.
WebKit Commit Bot
Comment 11 2017-02-17 14:41:27 PST
Comment on attachment 301836 [details] Patch v2 Clearing flags on attachment: 301836 Committed r212578: <http://trac.webkit.org/changeset/212578>
WebKit Commit Bot
Comment 12 2017-02-17 14:41:32 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.