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

feat: disable 'sendall' when user focuses amount #688

Merged
merged 22 commits into from Jan 16, 2019

Conversation

@kalgoop
Copy link
Contributor

commented Dec 8, 2018

Proposed changes

Send all option should be disabled when user puts the cursor on amount field.
I think, this fixed #682 .
However, I don't agree with @zillionn that amount should be reset when sendAll is disabled.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

Also, I have changed name of 'onSendAll' function to 'setSendAll', since this function may be used from other places than send all button.

kalgoop added some commits Dec 8, 2018

change function name: 'onSendAll' -> 'setSendAll'
Since, this function may be used from other places (other than send all button), this name is better.

zillionn pushed a commit to zillionn/desktop-wallet that referenced this pull request Dec 10, 2018

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

@kalgoop I sort of agree with @Zillion. Could you make it so that when disabling send all, it resets the amount if the amount === the value it would be if send all was enabled please. E.g. if i enable send all and then change the value manually, disabling shouldn't reset the value. if i enable send all, don't change the value and then disable send all, it should reset the value. Let me know if that doesn't make sense please.

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

What do you mean by reset ?
Reset to 0 or reset to value that existed before enabling 'send all'

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I think reset the field to an empty value so it's more obvious that it's been reset. Both empty and 0 give the same error so don't need to worry about that

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

IMO reset should be the initial value. Example:

  • I click on a Pay with ARK link to pay somebody 8.50 ARK
  • Wallet will populate all the fields for me
  • Enabling Send all will set the amount to my total amount
  • Disabling Send all should reset to the initial value - 8.50, not to 0
@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I'm happy with that @zillionn, I didn't consider the URI's loading in data to the send form.

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

ok. will push after some time

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

With above commit-
If the amount is initially empty, sendAll enabled and disabled -> amount remains the sendAll amount only
If the amount has some value initially, sendAllenabled and disabled -> amount resets to initial value

Is this Ok ?

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I think keep it as the second point. whatever the initial value was, keep that if resetting. If the form was opened with no amount, reset to no amount. If the form started with an amount, reset to that amount. but only reset if the amount didn't change between enabling and disabling send all

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

I'm unable to set field to blank value (or 0) due to validators.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Maybe try changing the validators so you can

@Zillion

This comment has been minimized.

Copy link

commented Dec 12, 2018

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

sorry about that @Zillion with one n 😅

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

@Zillion Give me your GitHub password and we solve this 😉

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@alexbarnsley setting amount to blank isn't possible due to the current v-model.
I was trying since hours to fix this, but couldn't.

This may be fixed by removing v-model attribute and validating those things on our own. But that would increase the size of code, in my opinion.

Btw, why do you think current implementation isn't better?
I feel current implementation is better.
Why user would prefer blank field after enabling and disabling sendAll ?
I think there will be more users that would want sendAll amount there after enabling and disabling sendAll with initial blank amount.

kalgoop and others added some commits Dec 13, 2018

@kalgoop kalgoop changed the title Disable 'sendall' when user focuses amount feat: disable 'sendall' when user focuses amount Dec 19, 2018

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

@alexbarnsley don't you think we should take feedback from users if they feel above is intuitive to them ?

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2018

@kalgoop Switching on/off should not break your form and should return to the previous value. Current implementation is broken, If you change the amount while the switch is on, it MUST go off, because currently it stays on and you can't change the amount.

Most people will probably never use the switch, but here is an example:

  • You click on ARK Pay link, popup will open and most of the fields populated including some amount, then you click Send All just to see the total amount in your wallet, because there is no such info in that popup, or misclick it or just out of curiosity, and then what, how you get back the amount from the ARK Pay link?! You have to close the popup, search for that exact same link and click it again.
@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I don't think it should disable Send All on focus. It should be when the value is changed

kalgoop added some commits Jan 6, 2019

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@alexbarnsley I dont agree. When user focuses, he'll come to know that sendAll has been disabled.
If sendAll is disabled on change, then even on focus sendAll is enabled and user may not even try changing the field.

Anyways, lint error and conflicts have been resolved.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Well it seems you disagree with everything I say so I'm going to ask others to review this.

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@alexbarnsley well ,I have given an explanation why I disagree.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

How is that rude? I gave my opinions to your changes and you've disagreed with them. I can't move forward with this PR with a stalemate between us, so I will get other people's feedback on your changes.

@kalgoop

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@alexbarnsley "disagree with everything I say ".
Okay, leave that.

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Jan 6, 2019

Personally, I'd say the "send all" button should not be toggled on focus, but when the user starts editing. If I now toggle "send all" and it fills the field, and then focus on it, it will no longer be on "send all" so I might get confused whether it's sending all or not. The moment the user starts editing the field after pressing "send all", it should toggle as the user is no longer sending all of his / her funds

Also, if you had nothing in the amount field and then toggle the "send all" button, it will fill with the maximum amount but not revert to an empty field afterwards (instead it will still have the full amount in the field). Might be something to look into too 🤔

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

So guys, how we move forward this PR?

The behaviour described by @ItsANameToo is what I would do.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@j-a-m-l we'll wait 14 days for a response and then close if there isn't one.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 14, 2019

Codecov Report

Merging #688 into develop will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #688      +/-   ##
===========================================
- Coverage    38.89%   38.85%   -0.05%     
===========================================
  Files          196      196              
  Lines         4833     4839       +6     
  Branches       945      945              
===========================================
  Hits          1880     1880              
- Misses        2835     2841       +6     
  Partials       118      118
Impacted Files Coverage Δ
...action/TransactionForm/TransactionFormTransfer.vue 5.42% <0%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd9219e...cd2c943. Read the comment docs.

kalgoop added some commits Jan 14, 2019

@j-a-m-l
Copy link
Contributor

left a comment

Thanks @kalgoop

@j-a-m-l j-a-m-l merged commit 2334d2f into ArkEcosystem:develop Jan 16, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

@kalgoop kalgoop deleted the kalgoop:patch-2 branch Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.