<rdar://51698385>
Created attachment 408000 [details] Patch
Created attachment 408001 [details] With Patch 408000
Comment on attachment 408000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408000&action=review r=me > Source/WebInspectorUI/ChangeLog:9 > + to be correctly formatted for the locale. Nit: we don't indent into paragraphs in Changelogs. > Source/WebInspectorUI/ChangeLog:19 > + bytes into localized strings. Ditto. > Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85 > + font-variant-caps: petite-caps; Hah, nice.
Comment on attachment 408000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408000&action=review >> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85 >> + font-variant-caps: petite-caps; > > Hah, nice. Why are we doing this? I don't think macOS uses petite-caps anywhere.
Comment on attachment 408000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408000&action=review >>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85 >>> + font-variant-caps: petite-caps; >> >> Hah, nice. > > Why are we doing this? I don't think macOS uses petite-caps anywhere. Keeps us much closer to consistent to how the size used to look before the change when it was in a separate element with `font-size: 15px`.
Honestly I hadn't heard of petite-caps. Having designed this part of the UI (for good or bad!) that definitely what what I was going for =)
Comment on attachment 408000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408000&action=review > Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:171 > + return WI.UIString("%.0f B").format(bytes).toLowerCase(); Seems this becomes a variant of `Number.bytesToString` in Utilities.js... Perhaps we should extend Number.bytesToString to allow for a KB threshold or skipping B, which at a quick glance seems to be the only difference I see. This function pushes for "0.N KB" instead of "NNN B" in the 103-1024 range. I believe I did this strategy of keeping to KB most of the time because if you up/down through a bunch of similarly sized resources around the 1KB range, having the UI change types between B / KB is not as nice as generally keeping it showing in terms of KB.
Created attachment 408010 [details] Patch
Comment on attachment 408000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408000&action=review >>>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85 >>>> + font-variant-caps: petite-caps; >>> >>> Hah, nice. >> >> Why are we doing this? I don't think macOS uses petite-caps anywhere. > > Keeps us much closer to consistent to how the size used to look before the change when it was in a separate element with `font-size: 15px`. Oh, I see. I slightly misunderstood its purpose initially. Looks good!
Created attachment 408011 [details] Patch
Comment on attachment 408011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408011&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1356 > + if (bytesThreshold === undefined) > + bytesThreshold = 1024; you can use logical assignment operators to do this :) ``` bytesThreshold ??= 1024; ``` alternatively you could use a default parameter ``` value (bytes, higherResolution, bytesThreshold = 1024) ``` but we usually prefer doing the fallback inside the function itself > Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:170 > + return Number.bytesToString(bytes, false, 103).toLowerCase(); I don't think `toLowerCase` is a good idea with a `WI.UIString`. You're making this value `petite-caps` anyways, so why do we need to lowercase it? NIT: when calling functions in other files with literal values, we usually like to create `const` variables right above so that it's easier to see the intended use of the literal value without having to find where the function is defined (which isn't always easy) ``` const higherResolution = false; const bytesThreshold = 103; return Number.bytesToString(bytes, higherResolution, bytesThreshold); ```
Comment on attachment 408011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408011&action=review >> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:170 >> + return Number.bytesToString(bytes, false, 103).toLowerCase(); > > I don't think `toLowerCase` is a good idea with a `WI.UIString`. You're making this value `petite-caps` anyways, so why do we need to lowercase it? > > NIT: when calling functions in other files with literal values, we usually like to create `const` variables right above so that it's easier to see the intended use of the literal value without having to find where the function is defined (which isn't always easy) > ``` > const higherResolution = false; > const bytesThreshold = 103; > return Number.bytesToString(bytes, higherResolution, bytesThreshold); > ``` `petite-caps` only affects lower case letters. The other possible petite option, `all-petite-caps` will affect uppercase and lowercase letters, but also the numbers... We would just use full sized letters here, and it wouldn't look too bad; just different.
Created attachment 408013 [details] If we used full size letters.
Created attachment 408022 [details] Patch
Comment on attachment 408022 [details] Patch r=me
Committed r266735: <https://trac.webkit.org/changeset/266735> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408022 [details].