[44.5][calvinfroedge][3][startailcoon] Withdraw Modal

wmbutler commented 7 years ago

Notes

[x] Symbol/Asset is a dropdown + typeahead. Only assets with (a) balances (b) gateways, open.* etc (c) BTS would appear in this list.
[x] The gateway cannot be chosen by the end user. It is automatically set based upon the asset choice.
[x] Estimated value is based upon the users preferred currency choice.
[ ] Address is validated per currency (is the gateway responsible for this validation)?
[x] If the user chooses to withdraw BTS, the gateway field does not need to be displayed.
[x] Available balance is clickable
[x] Available balance is white unless the value in the quantity box > available balance (with fees taken into consideration)
[x] User should be able to enter into estimated value if desired

screen shot 2017-10-15 at 9 46 14 pm

wmbutler commented 7 years ago

[ ] Confirmation box should be the same style as the BTS Send confirmation. #556

landry314 commented 7 years ago

I wouldn't call it "Symbol", I think "Coin" or "Currency" is better.

btsfav commented 7 years ago

we trade more than coins or currency, so I think symbol is fine. how do other trading platforms name the assets?

landry314 commented 7 years ago

But you can't withdraw tokens or bitAssets here, only digital coins held by a gateway.

Symbol is fine. I prefer Coin a little bit more.

btsfav commented 7 years ago

EOS / OMG are tokens for example

landry314 commented 7 years ago

oh, you are right!

diogogomes commented 6 years ago

@btsfav

we trade more than coins or currency, so I think symbol is fine. how do other trading platforms name the assets?

What do you think about replace Symbol with Asset?

calvinfroedge commented 6 years ago

@svk31 Is there a typeahead component which already exists in the codebase? Otherwise I can make one but don't want to duplicate work.

calvinfroedge commented 6 years ago

So I understand what the modal is supposed to look like, but there are some changes here before we get to that point...

Basically the step before the modal will have to change a bit, right? Do we know exactly how that part of the UX will change?

@wmbutler

wmbutler commented 6 years ago

@calvinfroedge you can leave that screen as is. Your modal will be called separately. I don't think we can just remove this screen. As a matter of fact, we are going to want to keep this for a period of time as a fallback option.

I'm thinking your new modal will be like Withdraw - beta

calvinfroedge commented 6 years ago

I need to bump this to next sprint @wmbutler

calvinfroedge commented 6 years ago

@wmbutler @startailcoon Please let me know how this is looking on the typeahead...

typeahead

@svk31 Should the onSelect just return the symbol? i.e. OPEN.BTC?

wmbutler commented 6 years ago

@calvinfroedge add needed hours to this.

wmbutler commented 6 years ago

@calvinfroedge please try to have this out by Jan 4. This is an important feature. Add enough hours to represent the time you put into it.

startailcoon commented 6 years ago

Spent aprox. ~2 hours on this issue, but it still needs more work before it's done.

startailcoon commented 6 years ago

@calvinfroedge are you doing work on this or do you wish me to dive into it further?

calvinfroedge commented 6 years ago

@startailcoon I will continue working on it

wmbutler commented 6 years ago

@calvinfroedge release date is 1/15. Hope you are able to have this by the 13th because this will need extensive testing. We have ample budget so go to it and keep your hours. This is a big one.

wmbutler commented 6 years ago

Notes

[x] Only show assets in the dropdown if your account contains > 0 of that asset
[ ] Limit dropdown contents to 5 in the typeahaed. (Comment by Calvin: This will probably be the case once you start typing now, as it only shows assets you own)
[ ] Typeahead should display as a menu below input field (This will be the case as it was only appearing higher because of the number of results)

test

[ ] Estimated value is failing to populate (See comment below regarding market depth)

withdraw

[x] Fixed height and width on the modal (see deposit modal for example.
[x] Header on Withdraw Modal (see deposit modal for example.

calvinfroedge commented 6 years ago

Estimated value will fail I think if there is not sufficient market depth. Look at BTC, ETH, etc.

On Jan 27, 2018 10:45 AM, "Bill Butler" notifications@github.com wrote:

Notes

Only show assets in the dropdown if your account contains > 0 of that asset

Limit dropdown contents to 5 in the typeahaed.

Typeahead should display as a menu below input field

[image: test] https://user-images.githubusercontent.com/1254810/35473462-9d9920e8-0346-11e8-999f-e343e06b6f40.gif

Estimated value is failing to populate

[image: withdraw] https://user-images.githubusercontent.com/1254810/35473444-6cb10b62-0346-11e8-92b6-83bf21ec6b85.gif

Fixed height and width on the modal (see deposit modal for example.

Header on Withdraw Modal (see deposit modal for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitshares/bitshares-ui/issues/588#issuecomment-360993196, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmoalLgKXRQoMDaYdfW9Xz-eO66RUJUks5tO0STgaJpZM4P6AQA .

startailcoon commented 6 years ago

A few issues I've found

[x] Selecting an asset that doesn't work (no valid gateway?) displays nothing. Should show an error.
[x] Selecting KRM doesn't view anything. This asset should be available as both gateways are working in the legacy withdraw.
[x] Should BTS be listed for withdrawal?
[x] Display warning when Quantity is larger than available.
[x] Move "Withdraw To Address" error to right margin abow input (see Buy Modal)
[x] Don't enable Withdraw button if there is an error, like Address or Quantity.
[x] Fee doesn't update on Memo input.
[x] Mimic style of the other modals with padding and logo

On Submit

[x] Cancel on unlock or confirm should bring back the Withdraw Modal with the inputted data

Issues removed / commented by calvin:

"Estimated value" field shouldn't be editable (This is desired behavior as per @wmbutler)
Clicking the arrow on Gateway doesn't open the list, clicking in the dropdown does. (See comment below, would like to handle this as separate issue @wmbutler )
Fee dropdown goes beyond screen when the height is not enough to display all available. (User can scroll, do not see this as an issue)
Add padding between Fee and Gateway Fee when screen is narrow. (Made screen very narrow, i.e. Iphone 4 width, and do not see that there is an issue)
View Withdraw button on dashboard for each withdrawable asset to the new modal (Would be nice if we can handle this as a separate issue)

calvinfroedge commented 6 years ago

@startailcoon

Should BTS be listed for withdrawal?

I think BTS should be listed for withdrawal, as at least at some other places have supported BTS. Sending BTS from one user to another feels very different from "withdrawing" to an external exchange like polo.

"Estimated value" field shouldn't be editable

This was part of the original request specified by @wmbutler (That estimated value should be specifiable by the user). This makes sense especially for assets that don't have sufficient market depth to give a quote.

Display warning when Quantity is larger than available.

The field is highlighted in red, I agree adding an error is a good idea.

Fee doesn't update on Memo input.

Should it? Does adding a memo increase the fee?

Thanks for feedback!!! Good ideas!

wmbutler commented 6 years ago

@calvinfroedge the fee is variable based on the contents of the memo. That's how we keep the blockchain from getting spammed with massive memos.

calvinfroedge commented 6 years ago

@wmbutler Understood!

startailcoon commented 6 years ago

Unless Calvin notify us, I will pick up this task for the next milestone.

wmbutler commented 6 years ago

@startailcoon please do. I've reached out to Calvin three times in the past week with no response.

startailcoon commented 6 years ago

@calvin notified me on friday that he would submit changes/updates to this still. I'm still waiting for these changes.

I'm still able to take over if necessary but will require some notification time if so.

calvinfroedge commented 6 years ago

Working on this all day today

calvinfroedge commented 6 years ago

@startailcoon

"Clicking the arrow on Gateway doesn't open the list, clicking in the dropdown does."

This is actually a complex issue. The problem is we're using an actual HTML dropdown for the gateway selection, and it's literally impossible to open a select programmatically.

We need to implement a custom select (kind of like the typehead, but obviously different) or we need to host that icon somewhere that we can use it in a stylesheet as the select background image. I kind of think it should be a separate issue as I can see spending several hours on this very stupid issue.

calvinfroedge commented 6 years ago

@wmbutler @startailcoon @svk31

Many of the outstanding issues were closed. A few that remain I would really prefer if we could handle separately. Hours updated.

startailcoon commented 6 years ago

Adding 1 hour for review

wmbutler commented 6 years ago

[x] User needs to be able to click on the actual icon in the typeahead (@AustinAmoruso we have a number of needed fixes on this. Since you created it, can you please take a look at this ASAP) ?
[x] Value in BTS is not calculated properly for RUDEX.STEEM. I entered .5 STEEM and the the BTS value is calculated at .078 BTS. Should be closer to say, $7.80. Maybe just a precision issue?
[x] When the withdraw modal is chosen, there is a long lag in bringing up the modal. Can you instead hide the right menu, present the modal, and then show a loading indicator? This would manage expectations better.

calvinfroedge commented 6 years ago

@wmbutler

1) User needs to be able to click on the actual icon in the typeahead -> I added a fix for this on my branch, it's just adding the click event to the chevron icon

2) Value in BTS is not calculated properly for RUDEX.STEEM -> The problem here was for assets with precision < 0

calvinfroedge commented 6 years ago

@wmbutler I've fixed the problem regarding loading time required for assets in the typeahead, so I think that resolves the third point.

wmbutler commented 6 years ago

One last comment in the PR regarding precision and estimation. I think we should leave the estimated value input field out for now. It's inaccurate in a variety of situations.

wmbutler commented 6 years ago

[x] Not properly warning user of insufficient balance

wmbutler commented 6 years ago

[ ] Incorrectly stating that there are no gateways eligible for this withdrawal when OL is clearly available.

wmbutler commented 6 years ago

[x] Withdraw modal takes about 5 seconds to appear. What is causing this? Can you background this operation so the user doesn't have to wait for the modal?

calvinfroedge commented 6 years ago

@wmbutler

I resolved the issue about balance warning, was also due to precision issue. I think your other two issues must both be related to calling the gateways.

wmbutler commented 6 years ago

We are calling OL endpoint in triplicate.

calls

wmbutler commented 6 years ago

calls

calvinfroedge commented 6 years ago

Extra network calls should no longer be happening...please check your local branch to see if that resolved your load lag issue

wmbutler commented 6 years ago

[x] Placeholder should say "Search for Asset to withdraw"

screen shot 2018-02-28 at 10 40 02 am

wmbutler commented 6 years ago

[ ] No gateways available to handle this withdrawal bug still exists. You must own OPEN.BTC to test this.

AustinAmoruso commented 6 years ago

@wmbutler Sorry for the delay on responding I will take a look at this tomorrow to fix the interaction issue.

calvinfroedge commented 6 years ago

I think it's fixed already

On Feb 28, 2018 2:40 PM, "Austin Amoruso" notifications@github.com wrote:

@wmbutler https://github.com/wmbutler Sorry for the delay on responding I will take a look at this tomorrow to fix the interaction issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitshares/bitshares-ui/issues/588#issuecomment-369375341, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmoajSpFeFb6H9L3pQn1YlWpMHuLT85ks5tZbnNgaJpZM4P6AQA .

AustinAmoruso commented 6 years ago

Well then the first rule of IT has worked in my favor wait to see if the issue resolves itself. But honestly I'm sorry I was so slow to respond will try to be more expedient in the future. @calvinfroedge Thank you for the heads up.

wmbutler commented 6 years ago

@AustinAmoruso thanks for checking back in. I believe you have @startailcoon or @ahdigital to thank for that.

bitshares / bitshares-ui