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.

Sign up for GitHub

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

Jump to bottom

feat: add 'Max' button to sending on-chain screen #1931

Merged
merged 1 commit into from
Feb 13, 2024
Merged

feat: add 'Max' button to sending on-chain screen #1931

merged 1 commit into from
Feb 13, 2024

Conversation

Restioson
Copy link
Contributor

@Restioson Restioson commented Jan 31, 2024 •
edited by bonomat
Loading

Proof of concept for #1925; major issue is how this should be handled in the amount field (see #1925 (comment)). This is one possible solution but isn't the greatest as

  1. If the fee increases it'd be over the amount again
  2. It doesn't actually use the draining logic

That being said, it is still editable by the user instead of being an entirely new mode of interaction, which is a plus.

Resolves #1925

@bonomat
Copy link
Contributor

bonomat commented Jan 31, 2024

Proof of concept for #1925; major issue is how this should be handled in the amount field (see #1925 (comment)). This is one possible solution but isn't the greatest as

1. If the fee increases it'd be over the amount again

2. It doesn't actually use the draining logic

That being said, it is still editable by the user instead of being an entirely new mode of interaction, which is a plus.

Mhm, I would just leave the logic with 0 and add a Max button. If the Max button is clicked, the input field is not editable and it says Max. Then we can keep the logic with 0 in the backend.

@bonomat bonomat mentioned this pull request Jan 31, 2024
Closed
@Restioson
Copy link
Contributor Author

Restioson commented Feb 1, 2024 •
edited
Loading

Mhm, I would just leave the logic with 0 and add a Max button. If the Max button is clicked, the input field is not editable and it says Max. Then we can keep the logic with 0 in the backend.

Maybe it can be some kind of toggle - when pressed, it becomes surrounded by blue and enabled. When tapped again, the background becomes white, unlocking the edit field. Otherwise, it could be a regular toggle/switch.

@Restioson
Copy link
Contributor Author

image
image

I changed the default amount to 1000 and added an 'enter an amount' error when the amount is zero - this is to prevent unticking max showing an error immediately

@bonomat
Copy link
Contributor

bonomat commented Feb 2, 2024

I changed the default amount to 1000 and added an 'enter an amount' error when the amount is zero - this is to prevent unticking max showing an error immediately

I like it.

@Restioson Restioson requested a review from bonomat February 5, 2024 08:10
bonomat
bonomat reviewed Feb 6, 2024
View reviewed changes
mobile/lib/features/wallet/send/send_onchain_screen.dart Outdated
_controller.text = _amount.formatted();
Amount amt = widget.destination.amount;
amt = amt.sats == 0 ? Amount(1000) : amt;
logger.i(amt);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the logger is not needed 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.

Good catch, thanks!

bonomat
bonomat reviewed Feb 6, 2024
View reviewed changes
mobile/lib/features/wallet/send/send_onchain_screen.dart Outdated
@@ -163,7 +177,7 @@ class _SendOnChainScreenState extends State<SendOnChainScreen> {
keyboardType: TextInputType.number,
textAlign: TextAlign.center,
decoration: const InputDecoration(
hintText: "0.00",
hintText: "1000",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't the hintText be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, but we are setting it to 1000 as the default in any case

bonomat
bonomat approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

I haven't run it yet, but from what I see this should be fine.

@bonomat bonomat added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit 716d6df Feb 13, 2024
9 checks passed
@bonomat bonomat deleted the add-send-max branch February 13, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonomat bonomat bonomat approved these changes

Assignees

@Restioson Restioson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Send Max button in sending on-chain screen
2 participants
@Restioson @bonomat