Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to switch fiat currencies #14

Merged
merged 20 commits into from
Oct 9, 2023
Merged

Add option to switch fiat currencies #14

merged 20 commits into from
Oct 9, 2023

Conversation

anthony-robin
Copy link
Contributor

@anthony-robin anthony-robin commented Sep 11, 2023

Fixes #12

Currently, the dashboard only display prices in $ fiat currency. This PR adds a new option allowing to switch from $ to using accurate rate provided by Gnosis API.

Capture d’écran 2023-09-11 à 20 23 34

⚠️ This is a draft PR ⚠️

I made most of changes to handle multiple currencies by my React knowledges are not strong enough for me to make it reactive. For now a page refresh is still necessary to update the price.
Feel free to commit to this MR, pushing appropriate changes, and refactor my code if it does not follow the React best practices or rules <3

Co-Authored-By @NandyBa

@anthony-robin anthony-robin added the enhancement New feature or request label Sep 11, 2023
Copy link
Member

@NandyBa NandyBa left a comment

Choose a reason for hiding this comment

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

Issue:

Issue regarding conversions (normally the with the current euro / dollar rate the netValue must be 1.07 lower in euro than in dollar be the current return value is 1.03 upper. So it's signify a problem in conversion.
Please refer to my comments to understand the error and fix it
Capture d’écran 2023-09-16 à 01 39 58
Capture d’écran 2023-09-16 à 01 40 16

Note:

Regarding the fact that page need to be refresh to update the price don't worry. We are thankful for your participation and will help you getting something smooth

src/components/cards/main/SummaryCard.tsx Outdated Show resolved Hide resolved
src/components/cards/main/SummaryCard.tsx Outdated Show resolved Hide resolved
@anthony-robin
Copy link
Contributor Author

Thank you for the review ! I will have a deeper look to it and will be very happy to increase my skills on React by your reviews and advices :)

Copy link
Contributor Author

@anthony-robin anthony-robin left a comment

Choose a reason for hiding this comment

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

I updated the PR with your previous review and added some comment to my code on parts I'm not sure about. Could help me to clarify the interrogations I have ?

Thank you :)

src/components/layouts/SettingsMenu.tsx Outdated Show resolved Hide resolved
src/components/commons/fields/CurrencyField.tsx Outdated Show resolved Hide resolved
src/components/cards/main/SummaryCard.tsx Show resolved Hide resolved
src/components/cards/main/SummaryCard.tsx Show resolved Hide resolved
src/components/layouts/SettingsMenu.tsx Outdated Show resolved Hide resolved
@NandyBa
Copy link
Member

NandyBa commented Sep 27, 2023

Let me check

@NandyBa
Copy link
Member

NandyBa commented Sep 27, 2023

Update the following cards to take into account currency changed

  • WorthCard
  • RentsCard
  • PropertiesCard

Anthony Robin added 3 commits October 2, 2023 17:54
Fixes #12

Currently, the dashboard only display prices in `$` fiat currency.
This PR adds a new option allowing to switch from `$` to `€` using accurate rate provided by Gnosis API.
@NandyBa
Copy link
Member

NandyBa commented Oct 2, 2023

last 4 commits have are the result of a peer coding with @anthony-robin.

The currency switch is now dynamic and use store

@anthony-robin
Copy link
Contributor Author

Updated the PR to handle more components currency switch

src/components/cards/AssetCard.tsx Outdated Show resolved Hide resolved
@anthony-robin
Copy link
Contributor Author

Pushed an update to the AssetTable component I missed earlier.
Still need to address Nandy's note about querying too much Gnosis endpoint.

@anthony-robin
Copy link
Contributor Author

Addressed last Nandy's suggestion by passing rate from the parent to each asset components to avoid too many API calls.

I still have some hydratation issues showing up on the table view, I'm not sure what the issue is...

@NandyBa NandyBa self-requested a review October 8, 2023 19:37
@NandyBa
Copy link
Member

NandyBa commented Oct 8, 2023

My most recent commits have:

  • Fixed the type: number | undefined instead of Number
  • Resolved Next and React type conflicts by updating @types/react to the latest version: 18.2.25

@NandyBa NandyBa requested a review from jycssu-com October 8, 2023 20:31
@jycssu-com jycssu-com merged commit 04e9eb7 into develop Oct 9, 2023
1 check passed
@jycssu-com jycssu-com deleted the eur-converter branch December 11, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add euro fiat currency support
3 participants