Skip to content
This repository was archived by the owner on Feb 23, 2021. It is now read-only.

Conversation

@tanx
Copy link
Contributor

@tanx tanx commented Jun 29, 2018

Closes #231

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tested the packaged app and it restored all of my settings correctly 👍 very nice!

One thing I noticed was that I get a ton of repetitive "Syncing to chain..."notifications, I'm wondering if we should open a ticket for having a number next to repeated consecutive notifications rather than listing each notification individually

good one on adding the store tests as well :)

it('should be infinite for unknown rate', () => {
settings.fiat = 'eur';
const rate = helpers.calculateExchangeRate(100000, settings);
expect(rate, 'to match', //);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this ever be the case, that the exchange rate is unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but the user's machine might not have an internet connect to fetch a current exchange rate. So we need to handle this case and display something.

store = new Store();
});

describe('init()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

import AppStorage from '../../../src/action/app-storage';
import * as logger from '../../../src/action/log';

describe('Action App Storage Unit Tests', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 tests

@tanx
Copy link
Contributor Author

tanx commented Jul 2, 2018

One thing I noticed was that I get a ton of repetitive "Syncing to chain..."notifications, I'm wondering if we should open a ticket for having a number next to repeated consecutive notifications rather than listing each notification individually

Good point. I was also hoping the CLI log screen would display the block height. But seems like we only display logs from lnd, not the app's info/error logs.

@tanx tanx merged commit 691b3b0 into master Jul 2, 2018
@tanx tanx deleted the save-settings branch July 2, 2018 21:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants