Skip to content

fixed amount input field to properly interpret BTC#800

Merged
theborakompanioni merged 5 commits intodevelfrom
AmtInputFix
Aug 17, 2024
Merged

fixed amount input field to properly interpret BTC#800
theborakompanioni merged 5 commits intodevelfrom
AmtInputFix

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.

@amitx13 amitx13 self-assigned this Jul 19, 2024
0xSaksham
0xSaksham reviewed Jul 19, 2024
View reviewed changes
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
View reviewed changes
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

theborakompanioni
theborakompanioni reviewed Jul 21, 2024
View reviewed changes
theborakompanioni
theborakompanioni reviewed Jul 21, 2024
View reviewed changes
@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.

theborakompanioni
theborakompanioni reviewed Aug 4, 2024
View reviewed changes
theborakompanioni
theborakompanioni reviewed Aug 4, 2024
View reviewed changes
@editwentyone
Copy link

editwentyone commented Aug 4, 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.

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

editwentyone commented Aug 5, 2024

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

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Aug 5, 2024

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

editwentyone commented Aug 15, 2024

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
@0xSaksham @amitx13 @theborakompanioni
Updating #812 branch (#819)
48f7095 
* 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

Reviewers

@theborakompanioni theborakompanioni theborakompanioni approved these changes

@bhaveshg16 bhaveshg16 Awaiting requested review from bhaveshg16

@editwentyone editwentyone Awaiting requested review from editwentyone

+1 more reviewer

@0xSaksham 0xSaksham 0xSaksham left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@amitx13 amitx13

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: Fix the Behavior of Amount Input Field

4 participants

@amitx13 @editwentyone @theborakompanioni @0xSaksham