Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Add SMART_NUMBER formatter and make it default #109

Merged
merged 6 commits into from
Feb 25, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Feb 20, 2019

Motivation

From apache/superset#6916 and past encounters with number formatting, Superset's default D3's SI formatter works ok most of the time but have some too scientific outputs which confuses general users such as

  • Format billions as xxxG (G = Giga) instead of B which people associate with billions and are more familiar with.
    1,000,000,000 => '1G'
    P.S. Luckily, millions and Mega have the same M abbreviation.
  • Format floating point using micro (m) which can be confused with m for minutes when the users are unaware of the context.
    0.01 => '10m'
  • The SI formatter requires number of precision point as parameter. The default is 3. This cause unnecessary rounding for small numbers.
    123.45 => '123'

Superset users therefore often have to define custom formatting or in the billion case, have to tolerate it.

Changes

This PR addresses the issues above and provide more humanized outputs for these edge cases, so the default number formatter should work most of the time out-of-the-box. It will reduce the amount of custom formatting need and improve user experience.

  • 1,000,000,000 => '1B'
  • 0.01 => '0.01'
  • 123.45 => '123.45'

💔 Breaking Changes

  • Rename constants in NumberFormats changing _CHANGE to _SIGNED.
  • Default number format is now SMART_NUMBER instead of D3 .3~s.

🏆 Enhancements

  • Add new number format factory createSmartNumberFormatter.

@kristw kristw requested a review from a team as a code owner February 20, 2019 20:20
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #109   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          72     73    +1     
  Lines         861    888   +27     
  Branches      195    204    +9     
=====================================
+ Hits          861    888   +27
Impacted Files Coverage Δ
...ges/superset-ui-number-format/src/NumberFormats.ts 100% <100%> (ø) ⬆️
...et-ui-number-format/src/NumberFormatterRegistry.ts 100% <100%> (ø) ⬆️
...format/src/factories/createSmartNumberFormatter.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc6d31e...e31d54d. Read the comment docs.

@kristw kristw added #enhancement New feature or request reviewable Ready for review labels Feb 20, 2019
@mistercrunch
Copy link
Contributor

We should add a note to UPDATING.md when we bump this on the Superset side.

} = {},
) {
const { description, signed = false, id, label } = config;
const siFormatter = d3Format(`.3~s`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could move these outside the function so they aren't created every call? looks like they don't ever change so no reason not to?

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid change! one suggestion 💬

@ghost
Copy link

ghost commented Feb 25, 2019

There were the following issues with this Pull Request

  • Commit: 5dccdad
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: e48cfa5
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: db083e9
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request reviewable Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants