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

fixed amount input field to properly interpret BTC #800

Merged
merged 5 commits into from
Aug 17, 2024

Conversation

amitx13
Copy link
Contributor

@amitx13 amitx13 commented Jul 19, 2024

This PR fixes #799
Fixed the amount input field to correctly interpret 1.0 as 1 BTC. Previously, it was incorrectly treated as a much smaller value; for example, entering 0.1 was considered 0.00000001 BTC.

Copy link
Contributor

@0xSaksham 0xSaksham left a comment

Choose a reason for hiding this comment

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

This is the most important thing mentioned by edi. It works great now!

editwentyone
editwentyone previously approved these changes Jul 19, 2024
Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

fixed, thank you

@amitx13
Copy link
Contributor Author

amitx13 commented Jul 24, 2024

Hey @theborakompanioni, if you have a moment, could you please take a look? I added a test case for BitcoinAmountInput and used a slightly different approach compared to what was previously used in the tests. The reason for this change is that using await user.type(inputElement, '1.0') was interpreting 1.0 as 1, which ultimately resulted in sats being displayed. Instead, I used fireEvent to handle this scenario.

@amitx13
Copy link
Contributor Author

amitx13 commented Aug 3, 2024

I've updated the BitcoinAmountInput component to only accept . as the decimal separator. I understand that restricting users to one separator may not be ideal, but this approach effectively solves the issues with BitcoinAmountInput and its related test cases.

@editwentyone
Copy link

I've updated the BitcoinAmountInput component to only accept . as the decimal separator. I understand that restricting users to one separator may not be ideal, but this approach effectively solves the issues with BitcoinAmountInput and its related test cases.

so will there be a solution in the future where we can differ between , and . as the separator to be l11n friendly?

@amitx13
Copy link
Contributor Author

amitx13 commented Aug 4, 2024

so will there be a solution in the future where we can differ between , and . as the separator to be l11n friendly?

Well in the English number format, JavaScript only accepts . as the decimal separator. To use , as the separator, we would need to change the number format to Italian. However, even if we change the number format to Italian, JavaScript will still interpret , as . for parsing purposes. The issue arises with inputs like 1.0 or 1,0 because JavaScript interprets it as 1, which is correct generally but not in the context we need

@editwentyone
Copy link

ok got it, lets keep it at . for the time being

@theborakompanioni
Copy link
Collaborator

Okay, @editwentyone, as @amitx13 has argued, making it a text input only (as opposed to a number input) increases consistentcy between browsers, but lacks the ability to use browser-native up/down arrows.. I can see that this is a reasonable trade-off - what do you think?

@editwentyone
Copy link

absolutely. good idea, I bet no one uses the arrows anyways!

@amitx13
Copy link
Contributor Author

amitx13 commented Aug 16, 2024

absolutely. good idea, I bet no one uses the arrows anyways!

Then, the only thing holding up the PR now are the reviews.

@theborakompanioni theborakompanioni merged commit 072a419 into devel Aug 17, 2024
@theborakompanioni theborakompanioni deleted the AmtInputFix branch August 17, 2024 12:04
0xSaksham added a commit that referenced this pull request Aug 21, 2024
* fix: amount input field to properly interpret BTC (#800)

* feat: add SchedulerConfirmationModal component for starting scheduled sweep (#803)

* feat: add SchedulerConfirmationModal component for starting scheduled sweep

* removed dead code

* modal is centered and design changes

* used ConfirmModal and pre-written styles

* refactor: SchedulerConfirmationModal.tsx

minor production error

* build(deps): update dependencies (#813)

 @emotion/react          ^11.11.4  →  ^11.13.0
 @types/react             ^18.3.2  →   ^18.3.3
 formik                    ^2.4.5  →    ^2.4.6
 qrcode                    ^1.5.3  →    ^1.5.4
 react-bootstrap           ^2.9.2  →   ^2.10.4
 react-router-bootstrap   ^0.26.2  →   ^0.26.3
 react-router-dom         ^6.23.1  →   ^6.26.1
 husky            ^8.0.3  →    ^9.1.4
 i18next        ^23.11.4  →  ^23.13.0
 lint-staged     ^14.0.1  →   ^15.2.9
 prettier         ^3.2.5  →    ^3.3.3
 react-i18next   ^14.1.1  →   ^15.0.1

* refactor: align utxo list and modal components (#815)

* refactor: externalize UtxoIcon component and utxoTags function

* refactor: externalize UtxoConfirmations component

* refactor: reuse utxo icons in Jar details and UTXO list

* refactor(ui): simpler checkbox in utxo list

* refactor(send): vertically align balance

* refactor(send): tooltip for shortened addresses

* refactor(ui): externalize UtxoTags component

* ui(send): show considered UTXOs before performing transaction (#807)

---------

Co-authored-by: apX13_ <[email protected]>
Co-authored-by: Thebora Kompanioni <[email protected]>
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.

Bug: Fix the Behavior of Amount Input Field
4 participants