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

[UI] CurrencyEntryBox - hotfix for release #12380

Merged
merged 8 commits into from Feb 7, 2024

Conversation

soosr
Copy link
Collaborator

@soosr soosr commented Feb 7, 2024

This PR makes the CurrencyEntryBox release-ready on master. If #11966 fails to be ready, we will go with this solution, and finish that PR after the release (as that PR is better in terms of functionality).

Tests are very much welcomed.

adamPetho
adamPetho previously approved these changes Feb 7, 2024
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

tACK, it fixes #11745

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

  • BTC or USD being on the left or right side should reset after closing the dialog?:
    switch sides -> close the dialog -> USD is still on the left side

  • it is possible to paste an address in the BTC column, which then returns the amount of the maximumnumberofbitcoins in the column

    public const decimal MaximumNumberOfBitcoins = 20999999.9769m;

@soosr
Copy link
Collaborator Author

soosr commented Feb 7, 2024

BTC or USD being on the left or right side should reset after closing the dialog?:
switch sides -> close the dialog -> USD is still on the left side

No, it is correct.

it is possible to paste an address in the BTC column, which then returns the amount of the maximumnumberofbitcoins in the column

restored the same functionality as on master, amounts above the balance cannot be pasted.

@MarnixCroes
Copy link
Collaborator

did some basic testing.
so far, it works and good to go for release👍

@turbolay
Copy link
Collaborator

turbolay commented Feb 7, 2024

I can't write a multi-digits USD amount without using arrows

CleanShot.2024-02-07.at.08.28.36.mp4

It's annoying because most of the time you want to write a multi-digits usd amount

@adamPetho
Copy link
Collaborator

adamPetho commented Feb 7, 2024

@turbolay this is the exact bug this PR is supposed to fix. This PR fixes that on my side, Win11.
Are you sure you are on this PR's branch and not on master?

@soosr
Copy link
Collaborator Author

soosr commented Feb 7, 2024

@turbolay this is the exact bug this PR is supposed to fix. This PR fixes this on my side, Win11. Are you sure you are on this PR's branch and not on master?

I also tested it on Mac, @turbolay can you double check?

@turbolay
Copy link
Collaborator

turbolay commented Feb 7, 2024

Yeah sorry, didn't see my fetch failed... I don't know what I have lately I struggle to focus and make many mistakes.

The only "issue" I noticed with this PR (probably it's done on purpose, IDK) is that the spacing is not respected when there is focus on a box. When focus is lost, spacing gets respected again

@wieslawsoltes
Copy link
Collaborator

@soosr Tested on macOS, can't see any issues so far.

@soosr
Copy link
Collaborator Author

soosr commented Feb 7, 2024

The only "issue" I noticed with this PR (probably it's done on purpose, IDK) is that the spacing is not respected when there is focus on a box. When focus is lost, spacing gets respected again

The source of all problems (and why a proper implementation is so complex) is the spacing... especially applying it while the user edits the input. So that is why the spacing is removed when they edit, for now.

@soosr soosr merged commit 3bfb817 into zkSNACKs:master Feb 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants