feat(wallet): system settings screen #2200

mrfelton · 2019-05-14T18:06:15Z

Description:

Add new system settings screen for managing global config options.

Note: This PR implements the base functionality in a basic way. Still waiting for final designs.

Motivation and Context:

fix #1535
fix #1536
fix #2099

How Has This Been Tested?

Manually

Screenshots:

Types of changes:

Feature

Checklist:

My code follows the code style of this project.
I have reviewed and updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes where needed.
All new and existing tests passed.
My commits have been squashed into a concise set of changes.

coveralls · 2019-05-15T10:21:10Z

Coverage decreased (-0.02%) to 20.537% when pulling d9cd800 on mrfelton:feat/1535-settings-page into 1d25603 on LN-Zap:feat/settings.

korhaliv

Some feedback

electron/updater.js

renderer/components/Settings/Settings.js

renderer/components/Settings/messages.js

renderer/components/UI/Select.js

renderer/components/UI/TransactionFeeInput.js

renderer/components/UI/messages.js

renderer/containers/ModalStack.js

renderer/store/db.js

mrfelton · 2019-05-19T12:56:29Z

I've addressed all these issues @korhaliv . Ready for re-review.

korhaliv

Some more feedback

korhaliv · 2019-05-20T09:17:27Z

electron/updater.js

-      await autoUpdater.checkForUpdates()
+      updaterLog.info('Automatic Updates enabled')
+      this.isActive = true
+      autoUpdater.checkForUpdates()
      const oneHour = 60 * 60 * 1000


Let's extract this into the config ?

korhaliv · 2019-05-20T09:18:37Z

package.json

@@ -350,8 +350,10 @@
    "lodash.merge": "4.6.1",
    "lodash.partition": "4.6.0",
    "lodash.pick": "4.4.0",
+    "lodash.set": "^4.3.2",


Looks like set was added not in exact mode

korhaliv · 2019-05-20T09:22:41Z

renderer/components/Settings/SettingsMenu/SettingsMenu.js

+    const { isSettingsMenuOpen, closeSettingsMenu } = this.props
+    if (
+      this.menuRef &&
+      (this.menuRef && !this.menuRef.contains(event.target)) &&


redundant this.menuRef check

yep, removed

korhaliv · 2019-05-20T09:23:43Z

renderer/components/Settings/SettingsMenu/SettingsMenu.js

+      this.menuRef &&
+      (this.menuRef && !this.menuRef.contains(event.target)) &&
+      (this.buttonRef && !this.buttonRef.contains(event.target)) &&
+      isSettingsMenuOpen


I'd move this to be the first check since it's faster than contains

good call. done

korhaliv · 2019-05-20T09:32:21Z

renderer/components/Settings/SettingsMenu/SettingsMenu.js

+            {isSettingsMenuOpen ? <AngleUp width="0.6em" /> : <AngleDown width="0.6em" />}
+          </Flex>
+        </Flex>
+        <Box ref={this.setWrapperRef}>{isSettingsMenuOpen && this.renderSettingsMenu()}</Box>


Is isSettingsMenuOpen check is purposely applied to the inner render func and not to the wrapping Box ?

No! Updated.

korhaliv · 2019-05-20T09:37:04Z

renderer/reducers/locale.js

@@ -32,19 +32,21 @@ export const receiveLocale = (event, locale) => dispatch => {

 export const initLocale = () => async (dispatch, getState) => {


async can go looks like

Actually, setLocale is an async method so I've updated this to await it.

korhaliv · 2019-05-20T09:37:22Z

renderer/reducers/locale.js

-  const userCurrency = state.settings.fiatTicker
-  if (userCurrency) {
-    dispatch(setFiatTicker(userCurrency))
+  const currentConfig = settingsSelectors.currentConfig(state)


Same here. Added missing await

korhaliv · 2019-05-20T09:42:08Z

renderer/reducers/ticker.js

@@ -150,6 +137,8 @@ const tickerSelectors = {}

 tickerSelectors.currency = currencySelector
 tickerSelectors.tickerLoading = tickerLoadingSelector
+tickerSelectors.fiatTicker = fiatTickerSelector
+tickerSelectors.fiatTickers = fiatTickersSelector


Ideally i'd like to have this named differently. LIke fiatTicker and allFilatTicker or fiatTickerList

The ticker reducer is exceptionally bad in terms of its naming and this issue of pluralised vs singular is a recurring one throughout our code codebase. I know this has come up several times and I have given the same response every time - but I really don't want to get into renaming things as part of this PR.

We need to do a wider cleanup of all of our reducers and standardize a bunch of things like this.

korhaliv · 2019-05-20T10:51:28Z

utils/renameKeys.js

@@ -0,0 +1,10 @@
+const renameKeys = (keysMap, obj) =>


Could you add a desc please and/or some basic tests in order to know how to use this

This method was actually never used in the end so I have removed it completely

korhaliv · 2019-05-20T10:53:38Z

utils/difference.js

+ * @param  {Object} base   Object to compare with
+ * @return {Object}        Return a new object who represent the diff
+ */
+const difference = (object, base) => {


Ideally, some tests would be great for this kind of utils, as they may be tricky sometimes

Added a few basic tests for this.

fix LN-Zap#1535

In order to support flexible fees, use generic keys fast, medium, slow rather than specific time based keys. Fix LN-Zap#2099

korhaliv

Tested ACK 38a5650

mrfelton added the type: enhancement New feature or request label May 14, 2019

mrfelton added this to the v0.5.0-beta milestone May 14, 2019

mrfelton self-assigned this May 14, 2019

mrfelton force-pushed the feat/1535-settings-page branch from 14ed217 to 1ae8c27 Compare May 14, 2019 20:37

mrfelton requested a review from korhaliv May 14, 2019 20:56

mrfelton force-pushed the feat/1535-settings-page branch 2 times, most recently from f038212 to b4abd39 Compare May 15, 2019 10:04

korhaliv suggested changes May 18, 2019

View reviewed changes

mrfelton force-pushed the feat/1535-settings-page branch from b4abd39 to c41af9f Compare May 19, 2019 12:55

mrfelton requested a review from korhaliv May 19, 2019 12:56

mrfelton force-pushed the feat/1535-settings-page branch from c41af9f to a9d6ddb Compare May 20, 2019 09:24

mrfelton changed the base branch from next to feat/settings May 20, 2019 09:39

korhaliv suggested changes May 20, 2019

View reviewed changes

mrfelton force-pushed the feat/1535-settings-page branch from a9d6ddb to 7715c2c Compare May 20, 2019 11:48

mrfelton requested a review from korhaliv May 20, 2019 12:28

mrfelton force-pushed the feat/1535-settings-page branch from 40d38d5 to 099251e Compare May 21, 2019 18:07

mrfelton added 12 commits May 21, 2019 20:11

refactor: read default theme from config

7c29a47

refactor: add lodash.transform dependency

304d732

refactor: add lodash.set dependency

39a5fc0

refactor: split isObject into its own export

983feab

refactor: add difference utility method

e0f1ec4

refactor: rename Settings to SettingsMenu

fb2a6e6

refactor: add default settings for currency and locale

e5ce334

refactor: use initial value for Select

70a7dbe

refactor: ensure item.key is used for Select component

be79ebd

feat(wallet): system settings screen

643fcce

fix LN-Zap#1535

refactor: read fiat ticker from current config

2327b54

refactor: read theme from current config

977e184

mrfelton added 9 commits May 21, 2019 20:11

refactor: read address format from current config

03439eb

refactor: read target congs from current config

4b985fb

refactor: extract fetchSettings method

07bd600

feat(wallet): ability to toggle autoupdater

0aa3145

refactor: use flex to align content in DataRow

92e7d32

refactor: migrate old settings to config overrides

6441491

test(unit): update snapshots

8664457

chore(i18n): extract new language strings

6c63872

refactor: rename fee keys to be more flexible

b332357

In order to support flexible fees, use generic keys fast, medium, slow rather than specific time based keys. Fix LN-Zap#2099

mrfelton force-pushed the feat/1535-settings-page branch from 099251e to a471a92 Compare May 21, 2019 18:12

mrfelton changed the base branch from feat/settings to next May 21, 2019 18:13

mrfelton changed the base branch from next to feat/settings May 21, 2019 18:13

mrfelton force-pushed the feat/1535-settings-page branch from a471a92 to 38a5650 Compare May 21, 2019 18:17

korhaliv approved these changes May 21, 2019

View reviewed changes

mrfelton added 5 commits May 21, 2019 20:34

feat(wallet): support custom fee target confs

5add358

chore(i18n): extract new language strings

6f84ea2

refactor: use schedular in ZapUpdater

39abee4

refactor: extract autoupdate interval to global settings

4ddd60a

test: add unit tests for difference method

d9cd800

mrfelton force-pushed the feat/1535-settings-page branch from 38a5650 to d9cd800 Compare May 21, 2019 18:35

mrfelton added 2 commits May 21, 2019 20:49

chore: update yarn.lock

deeef2a

fix(deps): update bitcoinjs-lib to v5.0.4

b6b4fab

mrfelton merged commit c1c4f20 into LN-Zap:feat/settings May 21, 2019

mrfelton deleted the feat/1535-settings-page branch May 21, 2019 19:08

This was referenced May 21, 2019

Standalone settings screen #1535

Closed

Add ability to toggle autoupdate #1536

Closed

Refactor: fees config #2099

Closed

Allow customisation of fee #2284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallet): system settings screen #2200

feat(wallet): system settings screen #2200

mrfelton commented May 14, 2019 •

edited

Loading

coveralls commented May 15, 2019 •

edited

Loading

korhaliv left a comment

mrfelton commented May 19, 2019

korhaliv left a comment

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv May 20, 2019

mrfelton May 20, 2019

korhaliv left a comment

		@@ -32,19 +32,21 @@ export const receiveLocale = (event, locale) => dispatch => {

		export const initLocale = () => async (dispatch, getState) => {

feat(wallet): system settings screen #2200

feat(wallet): system settings screen #2200

Conversation

mrfelton commented May 14, 2019 • edited Loading

Description:

Motivation and Context:

How Has This Been Tested?

Screenshots:

Types of changes:

Checklist:

coveralls commented May 15, 2019 • edited Loading

korhaliv left a comment

Choose a reason for hiding this comment

mrfelton commented May 19, 2019

korhaliv left a comment

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

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

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

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

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

korhaliv left a comment

Choose a reason for hiding this comment

mrfelton commented May 14, 2019 •

edited

Loading

coveralls commented May 15, 2019 •

edited

Loading