Switch from numeral to numbro

[eradman]

numeraljs is no longer maintained, and incorrectly parses high-precision floats (such as 1.2e-7) as NaN.

What type of PR is this?

• Bug Fix

Description

How is this tested?

• viz-lib yarn tests
• manually

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Current behavior using numeral.js

New behavior using numbro.js

[codecov]

Codecov Report

Merging #6344 (53bab51) into master (acf77f8) will increase coverage by 0.03%.
Report is 5 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6344      +/-   ##
==========================================
+ Coverage   60.74%   60.78%   +0.03%     
==========================================
  Files         153      153              
  Lines       12513    12527      +14     
  Branches     1695     1701       +6     
==========================================
+ Hits         7601     7614      +13     
- Misses       4686     4687       +1     
  Partials      226      226              

see 3 files with indirect coverage changes

[eradman]

Frontend tests failed:

1. numbers formatted without commas
2. percent is off by 100

[justinclift]

You ok to fix those? 😄

[eradman]

I should be able to figure it out. Probably a difference in the interface for specifying number format

The broken tests were run by cd viz-lib && yarn test, which I mistakenly thought yarn test covered

[justinclift]

The broken tests were run by cd viz-lib && yarn test, which I mistakenly thought yarn test covered

Oh. That's news to me too. Learning something every day I guess. 😄

[eradman]

I fixed most of the tests. The failing tests are complaining about incorrect tooltips on the counter widgets

    -   "targetValueTooltip": "24,484,000",
    +   "targetValueTooltip": "24484000",

The tooltips do seem to be formatted correctly when I run a development server. I'll investigate further next week

[justinclift]

Awesome. Sounds like it's almost across the line. 😄

[eradman]

I have not stumbled across any problems while testing this manually, but this change is affects a very hot code path (parsing all numbers returned by the database, and all the number formatting for tables). We should keep poking at this for at least a week.

[justinclift]

We should keep poking at this for at least a week.

Sounds sensible. 😄

[eradman]

Found and fixed an off-by-100 error

Unfortunately touching one more line of code tripped the code coverage warning 🙄

[eradman]

I have tested a variety of queries and dashboards, so far it's looking good.

I'll aim to merge this tomorrow (August 15) unless someone has additional comments or concerns.

[justinclift]

Sounds good to me. 😄