Detect locale #490

willpiers · 2018-08-03T00:01:06Z

Closes #486

willpiers · 2018-08-03T03:04:01Z

public/grpc-client.js


+  ipcMain.on('get-locale', event => {
+    event.sender.send('locale', { response: { locale } });
+  });


wasn't sure if these lines should be here or in electron.js - feedback wanted

Yeah better in electron.js as the local isn't really used for grpc.

tanx

@willpiers thanks so much for your PR! I've added some comments. Let me know if those make sense to you :)

tanx · 2018-08-03T13:31:36Z

package.json

    "electron-log": "^2.2.14",
    "electron-updater": "^2.21.10",
    "grpc": "^1.10.0",
+    "locale-currency": "0.0.2",


Please generate the package-lock.json with a compatible npm@5.6.0 and rebase/squash the big diff. thanks

tanx · 2018-08-03T13:33:03Z

public/grpc-client.js


+  ipcMain.on('get-locale', event => {
+    event.sender.send('locale', { response: { locale } });
+  });


Yeah better in electron.js as the local isn't really used for grpc.

tanx · 2018-08-03T13:35:46Z

public/preload.js

+    !/^(lnd)|(unlock)|(log)|(get-locale)|(locale)|(open-url)[a-zA-Z_-]{0,20}$/.test(
+      event
+    )
+  ) {


Probably easier to just prefix with lnd_ for future ipc calls. I was planning to do the same and refactor other IPCs as well to tidy things up.

tanx · 2018-08-03T13:37:00Z

src/action/grpc.js

+    let response = await this._sendIpc('get-locale', 'locale');
+    return response.locale;
+  }
+


Probably better to handle this in the settings action as locale has little to do with grpc.

tanx · 2018-08-06T10:58:30Z

src/action/index.js

-observe(store, 'lndReady', () => {
+observe(store, 'lndReady', async () => {
+  const locale = await grpc.getLocale();
+  setting.setFiatByLocale({ locale });


This shouldn't be necessary if the settings action listens to the ipc event internally.

tanx · 2018-08-06T10:59:37Z

src/action/setting.js

-      throw new Error(`Invalid fiat currency: ${fiat}`);
-    }
-    this._store.settings.fiat = fiat;
+    this._store.settings.fiat = FIATS[fiat] ? fiat : DEFAULT_FIAT;


I would suggest keeping the check like it was and ...

tanx · 2018-08-06T11:01:44Z

src/action/setting.js

+  setFiatByLocale({ locale }) {
+    const fiat = LocaleCurrency.getCurrency(locale).toLowerCase();
+    this.setFiatCurrency({ fiat });
+  }


... if (!FIATS[fiat]) falling back to usd if the app does not support the currency yet. This will allow us to detect Euro and Pounds for now and add more currency support in the future (which requires a design change in the settings).

the ternary expression on line 21 does just that - if fiat is any of the supported fiats, it will be used. otherwise USD

tanx · 2018-08-06T11:02:30Z

test/integration/action/action-integration.spec.js

+      expect(store2.settings.fiat, 'to be', 'usd');
+    });
+  });
+


No need to add an integration test since we don't spin up electron here anyway. Unit tests should suffice for this feature.

willpiers · 2018-08-06T22:42:02Z

src/action/grpc.js

+   */
+  sendLocaleRequest() {
+    return this._sendIpc('get-locale', 'locale');
+  }


options here:

make _sendIpc public so it can be called from actions/setting

replicate logic of _sendIpc in actions/setting, and pass ipcRenderer into that file

leave this code here, which seems out of place

I would prefer 1 or 3, just for aesthetics and simplicity

willpiers · 2018-08-06T22:42:29Z

src/action/setting.js

+    let response = await this._grpc.sendLocaleRequest();
+    const fiat = LocaleCurrency.getCurrency(response.locale).toLowerCase();
+    this.setFiatCurrency({ fiat });
+  }


i'll add an annotation to this function

tanx · 2018-08-07T15:35:15Z

Closing in favor of #501

willpiers force-pushed the detect-locale branch from e874aec to a6dc6ea Compare August 3, 2018 00:46

willpiers commented Aug 3, 2018

View reviewed changes

tanx suggested changes Aug 6, 2018

View reviewed changes

willpiers added 3 commits August 6, 2018 11:54

Detect currency based on locale

4c3e4c7

Use async ipc methods and add tests

a82ca9f

Fix tests

3a1533b

willpiers force-pushed the detect-locale branch from a6dc6ea to 724aa4c Compare August 6, 2018 18:03

Refactor to move ipc handlers to more logical places

967527a

willpiers force-pushed the detect-locale branch from 724aa4c to 967527a Compare August 6, 2018 18:09

willpiers commented Aug 6, 2018

View reviewed changes

tanx closed this Aug 7, 2018

Detect locale #490

Detect locale #490

Uh oh!

Conversation

willpiers commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willpiers Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanx commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

willpiers Aug 6, 2018 •

edited

Loading