feat(numfmt): currency shortcuts support internationalization #3062

Gggpound · 2024-08-14T11:21:27Z

close #1803

This time supports quick keys for USD, RMB, and EUR. Other currencies will be added after internationalization.

Pull Request Checklist

Related tickets or issues have been linked in the PR description (or missing issue).
Naming convention is followed (do please check it especially when you created new plugins, commands and resources).
Unit tests have been added for the changes (if applicable).
Breaking changes have been documented (or no breaking changes introduced in this PR).

codecov · 2024-08-14T11:47:34Z

Codecov Report

Attention: Patch coverage is 5.00000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 27.44%. Comparing base (d127e02) to head (4d594f1).
Report is 3 commits behind head on dev.

Files	Patch %	Lines
packages/sheets-numfmt/src/menu/menu.ts	0.00%	10 Missing ⚠️
...s-numfmt/src/controllers/numfmt.menu.controller.ts	0.00%	6 Missing ⚠️
...mfmt/src/commands/commands/set-currency.command.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3062      +/-   ##
==========================================
- Coverage   27.44%   27.44%   -0.01%     
==========================================
  Files        1980     1980              
  Lines      104889   104907      +18     
  Branches    22643    22647       +4     
==========================================
+ Hits        28787    28788       +1     
- Misses      76102    76119      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-08-14T11:53:11Z

View Deployment

📑 Examples	📚 Storybook
🔗 Preview link	🔗 Preview link

lovasoa · 2024-08-14T11:54:33Z

packages/sheets-numfmt/src/base/const/CURRENCY-SYMBOLS.ts

+    [LocaleType.EN_US]: '$', // United States Dollar
+    CA: 'C$', // Canadian Dollar
+    GB: '£', // British Pound Sterling
+    JP: '¥', // Japanese Yen
+    IN: '₹', // Indian Rupee
+    AU: 'A$', // Australian Dollar
+    [LocaleType.ZH_CN]: '¥', // Chinese Yuan
+    [LocaleType.ZH_TW]: '¥', // Chinese Yuan
+    KR: '₩', // South Korean Won
+    [LocaleType.RU_RU]: '₽', // Russian Ruble
+    // Euro countries
+    AT: '€', BE: '€', CY: '€', EE: '€', FI: '€', FR: '€',
+    DE: '€', GR: '€', IE: '€', IT: '€', LV: '€', LT: '€',
+    LU: '€', MT: '€', NL: '€', PT: '€', SK: '€', SI: '€', ES: '€',


Is it normal that two-letters country codes are mixed with full locale strings like en-US ?

Here, currency types are represented using language codes instead of coutry codes (You can find specific language codes in the file located at packages/core/src/types/enum/locale-type.ts.

Yes, and that's a problem, because the currency depends on the country, but not on the language

lovasoa · 2024-08-14T12:01:49Z

packages/sheets-numfmt/src/commands/commands/set-currency.command.ts

@@ -28,14 +29,17 @@ export const SetCurrencyCommand: ICommand = {
    handler: async (accessor: IAccessor) => {
        const commandService = accessor.get(ICommandService);
        const selectionManagerService = accessor.get(SheetsSelectionsService);
+        const localeService = accessor.get(LocaleService);
+        const locale = localeService.getCurrentLocale();


Is this going to work for euros ?
Or will we have to wait until univer is translated into every european language ?
I liked using the browser's current regional settings, because it allowed this to work with languages univer is not (and will never be) translated into.

No, no, no. The European region is also a future direction for our services. It's just that we currently don't have enough manpower to support this work. Most of the internationalization resources in the project are provided by the community, and we are very grateful for their help.
Can we try adding an enumeration of LocaleType euro, but translate the text in English (Wait for the open source community to provide the corresponding language copywriting)?

If you want to implement this button in the toolbar by yourself, you can try overriding this menu configuration.
After loading the numfmt-plugin, you can reset the CurrencyMenuItem by calling the setMenuItem method in menu.service.ts.

The thing is, there are many many different languages in the "euro region". What would an euro locale contain?

Are you sure you don't want to decouple supported languages from supported currencies?

【there are many many different languages in the "euro region"】Could there be multiple languages for the same currency? That means that our localType enumerations may be excessive.
Please give me a little more time to discuss it internally

Ideally, I would like the default currency displayed to SQLPage users to be the currency of the place they are in, even if the rest of the UI is in English.

Could there be multiple languages for the same currency?

Yes. There are 20 countries in the euro zone, speaking 19 different languages. Some of these languages have <1M locutors. I can speak only one of these 19 languages (french). I understand you have grand ambitions, but univerjs will probably not be translated in all of these languages anytime soon. It would be great if we could still have the € sign by default...

Sure, I think providing an API to set a custom currency symbol is a great solution. It allows you to set the currency symbol independently of the platform language and Univer own language settings. When integrating with Univer, you can use the API to set the currency symbol, regardless of how you obtain the system language or coutry code.

I think there is a misunderstanding. I don't need an api to set the default currency symbol, I would like the default symbol to automatically reflect the users preference, wherever they are. That's what my initial pull request did.

We will provide an API for setting the currency symbol, and you can write some code to call it. For example, you can use the browser's user-agent to recognize the country code and set the corresponding symbol.

We will add a country code identifier to the snapshot upon creation. If not set, it will default to the mapped value of that country code and will not follow the language settings.

Later, we will provide a StackBlitz code to guide you through the implementation.😊

There are some blocking bugs that need to be resolved, and we expect to provide them to you by next week.
😊

wzhudev

Overall looks good to me. But there are some code quality issues.

wzhudev · 2024-08-16T11:36:08Z

packages/sheets-numfmt/src/controllers/numfmt.menu.controller.ts

@@ -29,6 +31,8 @@ export const DefaultSheetNumfmtConfig = {};

 @OnLifecycle(LifecycleStages.Rendered, NumfmtMenuController)
 export class NumfmtMenuController extends Disposable {
+    private _currencySymbol$ = new BehaviorSubject<keyof typeof currencyMap>('ZH_CN');


It is better to infer the default currency symbol for the browser's navgiator info whenever possible. Since this property is at UI layer, it is considered as harmless.

It it recommended to make property readonly whenever possible.

wzhudev · 2024-08-16T11:36:58Z

packages/sheets-numfmt/src/controllers/numfmt.menu.controller.ts

@@ -29,6 +31,8 @@ export const DefaultSheetNumfmtConfig = {};

 @OnLifecycle(LifecycleStages.Rendered, NumfmtMenuController)
 export class NumfmtMenuController extends Disposable {
+    private _currencySymbol$ = new BehaviorSubject<keyof typeof currencyMap>('ZH_CN');
+    public currencySymbol$ = this._currencySymbol$.asObservable();


Add a blank line before constructors to make the code clear to read.

packages/sheets-numfmt/src/index.ts

wzhudev · 2024-08-16T11:50:50Z

packages/sheets-numfmt/src/controllers/numfmt.menu.controller.ts

@@ -50,4 +54,12 @@ export class NumfmtMenuController extends Disposable {
        this.disposeWithMe((this._componentManager.register(MORE_NUMFMT_TYPE_KEY, MoreNumfmtType)));
        this.disposeWithMe((this._componentManager.register(OPTIONS_KEY, Options)));
    }
+
+    public setCurrencySymbol(symbol: keyof typeof currencyMap) {


JSDoc is expected to be required on this method if it is intended to be used directly by users. And a relate change on the Facade API is required.

Gggpound · 2024-08-17T02:59:29Z

After the Univer instance is created, execute the following code:

import { NumfmtMenuController, NumfmtMenuController } from '@univerjs/sheets-numfmt';
import { LifecycleService, LifecycleStages } from '@univerjs/core';

const univer = new Univer();

univer.registerPlugin(UniverSheetsNumfmtPlugin);

setCurrencySymbol()
function setCurrencySymbol() {
    const injector = univer.__getInjector();
    const lifecycleService = injector.get(LifecycleService);

    function calculateYourCurrencySymbol() {
         function getRegionCode(): string {
            // Get the user's locale
            const userLocale: string = navigator.language || (navigator as any).userLanguage;
            const regionCode: string = userLocale.split('-')[1] || userLocale.split('-')[0];

            // Return the corresponding currency symbol or default to USD
            return regionCode;
        }
        const numfmtMenuController = injector.get(NumfmtMenuController);
        // If it is not legal, it will be used ‘$’ by default
        numfmtMenuController.setCurrencySymbol(getRegionCode());
    }

    const dispose = lifecycleService.lifecycle$.subscribe((stage) => {
        if (LifecycleStages.Rendered === stage) {
            calculateYourCurrencySymbol();
            dispose.unsubscribe();
        }
    });
}

wzhudev

LGTM. Please add an example or some documentation on univer.ai to demonstrate how this works. @Gggpound

Gggpound requested review from jikkai, Jocs, ybzky, yuhongz, DR-Univer, Dushusir, wzhudev and siam-ese as code owners August 14, 2024 11:21

Gggpound mentioned this pull request Aug 14, 2024

feat: set the default currency based on the user's locale #2998

Closed

4 tasks

Gggpound force-pushed the feat-numfmt-0814 branch 3 times, most recently from f75d138 to 0b58ec0 Compare August 14, 2024 11:39

Jocs approved these changes Aug 14, 2024

View reviewed changes

lovasoa reviewed Aug 14, 2024

View reviewed changes

zhaolixin7 added the qa:verified This PR has already by verified by a QA and is considered good enough to be merge label Aug 14, 2024

Gggpound force-pushed the feat-numfmt-0814 branch from 0b58ec0 to bd100dd Compare August 15, 2024 13:16

wzhudev requested changes Aug 16, 2024

View reviewed changes

Gggpound force-pushed the feat-numfmt-0814 branch from bd100dd to e156571 Compare August 17, 2024 02:49

Gggpound force-pushed the feat-numfmt-0814 branch 2 times, most recently from 6977f13 to 590e272 Compare August 17, 2024 03:43

feat(numfmt): currency shortcuts support internationalization

4d594f1

Gggpound force-pushed the feat-numfmt-0814 branch from 590e272 to 4d594f1 Compare August 17, 2024 03:51

wzhudev approved these changes Aug 17, 2024

View reviewed changes

wzhudev merged commit 736cb9b into dev Aug 17, 2024
9 checks passed

wzhudev deleted the feat-numfmt-0814 branch August 17, 2024 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(numfmt): currency shortcuts support internationalization #3062

feat(numfmt): currency shortcuts support internationalization #3062

Gggpound commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

lovasoa Aug 14, 2024

Gggpound Aug 14, 2024 •

edited

Loading

lovasoa Aug 14, 2024

lovasoa Aug 14, 2024

Gggpound Aug 14, 2024

Gggpound Aug 14, 2024 •

edited

Loading

lovasoa Aug 14, 2024

lovasoa Aug 14, 2024

Gggpound Aug 14, 2024

lovasoa Aug 14, 2024 •

edited

Loading

Gggpound Aug 14, 2024 •

edited

Loading

lovasoa Aug 14, 2024

Gggpound Aug 15, 2024 •

edited

Loading

wzhudev left a comment

wzhudev Aug 16, 2024 •

edited

Loading

wzhudev Aug 16, 2024

wzhudev Aug 16, 2024

wzhudev Aug 16, 2024 •

edited

Loading

Gggpound commented Aug 17, 2024 •

edited

Loading

wzhudev left a comment •

edited

Loading

feat(numfmt): currency shortcuts support internationalization #3062

feat(numfmt): currency shortcuts support internationalization #3062

Conversation

Gggpound commented Aug 14, 2024 • edited Loading

Pull Request Checklist

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 14, 2024 • edited Loading

View Deployment

Choose a reason for hiding this comment

Gggpound Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gggpound Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovasoa Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Gggpound Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gggpound Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

wzhudev left a comment

Choose a reason for hiding this comment

wzhudev Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzhudev Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Gggpound commented Aug 17, 2024 • edited Loading

wzhudev left a comment • edited Loading

Choose a reason for hiding this comment

Gggpound commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

Gggpound Aug 14, 2024 •

edited

Loading

Gggpound Aug 14, 2024 •

edited

Loading

lovasoa Aug 14, 2024 •

edited

Loading

Gggpound Aug 14, 2024 •

edited

Loading

Gggpound Aug 15, 2024 •

edited

Loading

wzhudev Aug 16, 2024 •

edited

Loading

wzhudev Aug 16, 2024 •

edited

Loading

Gggpound commented Aug 17, 2024 •

edited

Loading

wzhudev left a comment •

edited

Loading