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

Currency Converter #1968

Merged
merged 18 commits into from
Feb 27, 2024
Merged

Conversation

shubhamkmr04
Copy link
Contributor

No description provided.

@kaloudis
Copy link
Contributor

kaloudis commented Feb 1, 2024

CurrencyConverter not CurrencyConvertor

@shubhamkmr04 shubhamkmr04 changed the title WIP: Currency Convertor WIP: Currency Converter Feb 2, 2024
<TextInput
style={styles.inputBox}
suffix={currency}
placeholder={`Enter amount in ${currency}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be localized. Although here maybe we should show the currency flag as well

@shubhamkmr04 shubhamkmr04 force-pushed the shubham/CurrencyConvertor branch 2 times, most recently from 80a634f to 8e6a711 Compare February 13, 2024 08:36
@@ -0,0 +1,156 @@
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this file or can we add in the functionality to SelectCurrency?

Copy link
Contributor Author

@shubhamkmr04 shubhamkmr04 Feb 13, 2024

Choose a reason for hiding this comment

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

Hmm, I'll figure out if we can without messing it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added this functionality in SelectCurrency and removed AddCurrencies

@kaloudis kaloudis added this to the v0.8.2 milestone Feb 13, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

are we using this file anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not right now, but I am thinking of using it in placeholder for BTC and SAT inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add it then

@kaloudis
Copy link
Contributor

I'm hitting some edge cases where I forms are unresponsive when I add them at first, sometimes. Also issues with the order not getting saved, after reordering fields.

@kaloudis
Copy link
Contributor

reordering is fixed now - but still having an issue with new fields not affecting the others

@kaloudis
Copy link
Contributor

nit: instead of writing it as SAT, let's spell out satoshis as sats

@kaloudis
Copy link
Contributor

Perhaps the country flag should always be displayed and be to the right of the currency code on the right side

@shubhamkmr04
Copy link
Contributor Author

Perhaps the country flag should always be displayed and be to the right of the currency code on the right side

got it

@shubhamkmr04
Copy link
Contributor Author

reordering is fixed now - but still having an issue with new fields not affecting the others

I didn't quite understand that bug.

@@ -170,15 +192,15 @@ export default class CurrencyConverter extends React.Component<
// Convert the value to other currencies and BTC
Object.keys(convertedValues).forEach((key) => {
if (key !== currency) {
let conversionRate: number | null = null;
let convertedValue = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

why the change from a number to a string here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revamped this function according to type string, its better this way also because we are using numberWithDecimals and numberWithCommas now. Working perfectly now with applying the formatting to the currencies.

@shubhamkmr04 shubhamkmr04 changed the title WIP: Currency Converter Currency Converter Feb 21, 2024
assets/images/SVG/bitcoin-icon.svg Outdated Show resolved Hide resolved
@kaloudis kaloudis merged commit 1fd5c73 into ZeusLN:master Feb 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants