Closed Bug 1239730 Opened 8 years ago Closed 8 years ago

Add spaces after every three digits to make numbers more readable

Categories

(DevTools :: Memory, defect, P2)

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: fitzgen, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 1 obsolete file)

Right now we have

> 1234567890

but it would be more readable if it was like

> 1 234 567 890
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
I imagine we'll have to tackle the un-tableness of the tree view soon as these numbers grow larger and larger in render area
Yeah, that is probably higher priority work than this, although neither blocks the other.
Attached patch bug1239730.diff (obsolete) — Splinter Review
Moved the format functions used in the different tree item classes to devtools/client/memory/utils.js and added a unit test.

try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=5899c19faad7
Attachment #8708120 - Flags: review?(jsantell)
Comment on attachment 8708120 [details] [diff] [review]
bug1239730.diff

Review of attachment 8708120 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/memory/components/census-tree-item.js
@@ +32,5 @@
>        showSign,
>        onViewSourceInDebugger,
>      } = this.props;
>  
> +    const bytes = formatNumber(item.bytes, showSign);

Are there any locales which do not have a delimiter every 3 digits? I know the delimiter can change, but wonder if this function should be localized somehow. Not too worried.

::: devtools/client/memory/test/unit/test_utils-format.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

These tests can probably be put into the other test utils file since those are all pretty much one line assertions

@@ +30,5 @@
> +  equal(utils.formatNumber(-12, true), "-12",
> +    "formatNumber can display number sign (negative)");
> +
> +  equal(utils.formatPercent(12), "12%", "formatPercent returns 12% for 12");
> +  equal(utils.formatPercent(12345), "12 345%",

I was going to say this looks weird for a %, but we shouldn't have > 100% anyway :)
Attachment #8708120 - Flags: review?(jsantell) → review+
Carry over r+ from previous review, merged back format tests to main utils test file.
Attachment #8708120 - Attachment is obsolete: true
Attachment #8709058 - Flags: review+
Previous try push had no related failure. 
New try push for reference : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c955fa558b91
Keywords: checkin-needed
(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #4)
> Comment on attachment 8708120 [details] [diff] [review]
> bug1239730.diff
> 
> Review of attachment 8708120 [details] [diff] [review]:

Thanks for the review!

> 
> I was going to say this looks weird for a %, but we shouldn't have > 100%
> anyway :)

I was wondering the same, but if we display an increase as %, we might have > 100% I guess?

> 
> These tests can probably be put into the other test utils file since those
> are all pretty much one line assertions

Ok done!

> Are there any locales which do not have a delimiter every 3 digits? I know
> the delimiter can change, but wonder if this function should be localized
> somehow.

I don't know either. I guess it's fine to always display it this way for now.
https://hg.mozilla.org/mozilla-central/rev/90e90ec9da96
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.