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

Add currency conversion setting #249

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

EddWills95
Copy link
Contributor

@EddWills95 EddWills95 commented Nov 1, 2020

  • Adds a currency selector for conversion and the logic to make the call to the manager in order to fetch the correct price
  • Currently supports: "USD", "EUR", "GBP" (but could obviously any)

image

image

Copy link
Contributor

@louneskmt louneskmt left a comment

Choose a reason for hiding this comment

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

In the whole code, "Currency"or "Fiat Currency" sounds better to me than "Conversion Currency".

I also would not use the window local storage to store this user preference, but the user file. See this. I'm working on a commit to do that.

@EddWills95 EddWills95 force-pushed the add-price-symbol-setting branch from 9b9a611 to d35a055 Compare November 1, 2020 18:54
@louneskmt
Copy link
Contributor

Just created 3 PRs relating to user settings : getumbrel/umbrel#302, getumbrel/umbrel-manager#58, #250.

Can you try use this code to set your Currency setting? I couldn't really test it, but It should work haha

@EddWills95
Copy link
Contributor Author

I think it makes sense to hang tight for this PR: #250
Then I can build on top of it

@@ -93,7 +93,8 @@ const API = {

const requestOptions = {
method: "get",
url
url,
params: data.params || {}
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for? I don't see where data is passed in to API.get(url, data) anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

He added it to specify currency for the price route, but finally he chose to use query parameters instead of a data body. We should just discard these changes now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

@louneskmt
Copy link
Contributor

Also @lukechilds, we have a little issue we could not yet figure out how to solve. On the page load or reload, it seems that the getPrice action and currency conversion occurs before the settings state is set. It leads for example to a $NaN wallet balance in the vertical menu.

I think it comes from the fetchData DashboardLayout method, possibly because the getPrice action resolves before the getSettings one, so it cannot correctly fetch the price as the currency setting is not yet defined in the state:

fetchData() {
this.$store.dispatch("system/getUnit");
this.$store.dispatch("user/getSettings");
this.$store.dispatch("bitcoin/getSync");
this.$store.dispatch("bitcoin/getBalance");
this.$store.dispatch("bitcoin/getTransactions");
this.$store.dispatch("lightning/getSync");
this.$store.dispatch("lightning/getTransactions");
this.$store.dispatch("lightning/getChannels");
this.$store.dispatch("bitcoin/getPrice");
this.$store.dispatch("system/getAvailableUpdate");
},

Maybe I'm wrong...

For the time being, I've set a return in case the currency is not defined:

const { currency } = rootState.user.settings;
if(!currency) return;

But honestly I don't think it's the good solution (and it doesn't really solve the problem haha).

What do you think?

EddWills95 and others added 2 commits November 6, 2020 17:52
change settings POST route

Co-authored-by: Lounès Ksouri <[email protected]>
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