feat: add duration formatter #209
Conversation
|
For context: We needed a queue time chart on a dashboard, and all existing number formatting options were sub-optimal. For example, 1 min 30 seconds would display as either "90" (seconds) or "1.5" (minutes). This uses pretty-ms to format either milliseconds or seconds into more human readable format:
Not sure if community PRs are welcomed yet, but thought this might be useful to others, too, so decided to open up a PR. |
|
Deploy preview for superset-ui ready! Built with commit e718a0b |
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 99.91% 99.91% +<.01%
==========================================
Files 111 112 +1
Lines 1239 1245 +6
Branches 303 307 +4
==========================================
+ Hits 1238 1244 +6
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for submitting. I think this is a nice addition to the library.
I have a few comments below.
Note: After this one eventually merges, it will be part of @superset-ui/number-format@0.12.x you may have to wait for updates to incubator-superset to upgrade from 0.11 to 0.12 which include a few breaking changes that need to be addressed.
packages/superset-ui-number-format/src/factories/createDurationFormatter.ts
Show resolved
Hide resolved
| @@ -3,6 +3,10 @@ const DOLLAR_SIGNED = '+$,.2f'; | |||
| const DOLLAR_ROUND = '$,d'; | |||
| const DOLLAR_ROUND_SIGNED = '+$,d'; | |||
|
|
|||
| const DURATION = 'DURATION'; | |||
There was a problem hiding this comment.
Please revert the changes to this file. The duration formatter should be optional and not registered by default. There are other apps using this package and some do not need duration formatter. You can register these formatters in incubator-superset's setupFormatter.js. That way other apps can take advantage of tree-shaking and won't have to bundle pretty-ms.
| @@ -14,6 +15,9 @@ export default class NumberFormatterRegistry extends RegistryWithDefaultKey< | |||
| overwritePolicy: OverwritePolicy.WARN, | |||
| }); | |||
|
|
|||
| this.registerValue(NumberFormats.DURATION, createDurationFormatter()); | |||
There was a problem hiding this comment.
Please revert the changes to this file. The duration formatter should be optional and not registered by default. There are other apps using this package and some do not need duration formatter. You can register these formatters in incubator-superset's setupFormatter.js. That way other apps can take advantage of tree-shaking and won't have to bundle pretty-ms when they are not using.
There was a problem hiding this comment.
You can add a section in README to instruct how to make duration formatter available.
There was a problem hiding this comment.
Ok that makes sense. I wasn't able to find setupFormatter.js or any reference to getNumberFormatterRegistry() in incubator-superset, but I think I have a good idea of how it should be done. So if it's ok I'll open a new PR to add instructions to README.md once 0.12 is released and durationFormatter gets implemented in the backend.
There was a problem hiding this comment.
Sorry I made a typo. It is setupFormatters.js. For README I meant packages/superset-ui-number-format/README.md
|
Thanks @kristw for the review and helpful comments. Before I go about making the proposed changes, do you think the name is good ( |
|
@villebro I think |
Updates the requirements on [@data-ui/histogram](https://github.com/williaster/data-ui) to permit the latest version. - [Release notes](https://github.com/williaster/data-ui/releases) - [Changelog](https://github.com/williaster/data-ui/blob/master/CHANGELOG.md) - [Commits](williaster/data-ui@v0.0.80...v0.0.81) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
馃弳 Enhancements
pretty-ms(MIT license).