You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/12/17 08:31:11 UTC
[GitHub] [superset] hbruch opened a new pull request #17796: feat: Create D3NumberFormatters with active Locale
hbruch opened a new pull request #17796:
URL: https://github.com/apache/superset/pull/17796
### SUMMARY
Currently, numbers and currencies are formatted using the en Locale, i.e. with group separator `,` and decimal separator `.`
There have been a couple of issues reports raising the need to customize superset and proposing ways to do so (e.g. #https://github.com/apache/superset/issues/3972 and https://github.com/apache/superset/issues/8913).
This (draft) PR suggests to apply the currently active Locale to do number Formatting (though this might not be the best solution, I'm open for feedback...)
### Open questions
While implementing this proposal, I had a number of questions:
1. Should number formats really change when changing the Locale, as just changing the unit without doing currency conversion is plain false (For currencies, adding a special Formatter as proposed in https://stackoverflow.com/questions/63319902/customise-the-number-format-in-apache-superset/63385549#63385549 might be the better way to do so...)
2. Should superset copy D3's locale defintions or just retrieve them from CDN (Note that a Locale conversion from superset to D3 would be necessary, as highlighted by @craigappl [here](https://github.com/apache/superset/issues/3972#issuecomment-447307711)
3. There are a couple of other places where numbers are formatted, which are not addressed by this PR (e.g. table chart)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
<img width="875" alt="grafik" src="https://user-images.githubusercontent.com/2187389/146511296-cbdff3ab-705e-44da-b493-55bb10f8b343.png">
After:
<img width="869" alt="grafik" src="https://user-images.githubusercontent.com/2187389/146511136-262970da-2d5b-4aca-8e3f-fcef6ca45b9f.png">
### TESTING INSTRUCTIONS
1. add "de": {"flag": "de", "name": "Deutsch"}, to the list of LANGUAGES in superset_config.py
2. start superset an switch to German (Deutsch)
3. display some charts and see ther number format changed.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #3972 #8913
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #17796: feat: Create D3NumberFormatters with active Locale
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #17796:
URL: https://github.com/apache/superset/pull/17796#issuecomment-996557529
# [Codecov](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17796](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8db8c2d) into [master](https://codecov.io/gh/apache/superset/commit/c5af7a48df24207839913a61d9aed31148697ee2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c5af7a4) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #17796 +/- ##
=======================================
Coverage 67.79% 67.79%
=======================================
Files 1605 1606 +1
Lines 64208 64210 +2
Branches 6790 6790
=======================================
+ Hits 43528 43530 +2
Misses 18824 18824
Partials 1856 1856
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `54.83% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-core/src/number-format/NumberFormatterRegistry.ts](https://codecov.io/gh/apache/superset/pull/17796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvbnVtYmVyLWZvcm1hdC9OdW1iZXJGb3JtYXR0ZXJSZWdpc3RyeS50cw==) | `100.00% <ø> (ø)` | |
| [...erset-ui-core/src/number-format/D3FormatLocales.ts](https://codecov.io/gh/apache/superset/pull/17796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvbnVtYmVyLWZvcm1hdC9EM0Zvcm1hdExvY2FsZXMudHM=) | `100.00% <100.00%> (ø)` | |
| [...set-ui-core/src/translation/TranslatorSingleton.ts](https://codecov.io/gh/apache/superset/pull/17796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdHJhbnNsYXRpb24vVHJhbnNsYXRvclNpbmdsZXRvbi50cw==) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c5af7a4...8db8c2d](https://codecov.io/gh/apache/superset/pull/17796?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] MM-Lehmann commented on pull request #17796: feat: Create D3NumberFormatters with active Locale
Posted by GitBox <gi...@apache.org>.
MM-Lehmann commented on pull request #17796:
URL: https://github.com/apache/superset/pull/17796#issuecomment-1063329666
Nice work. When are you ready to merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org