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

Take offer screen layout issue #1742

Closed
ManfredKarrer opened this issue Oct 2, 2018 · 5 comments
Closed

Take offer screen layout issue #1742

ManfredKarrer opened this issue Oct 2, 2018 · 5 comments
Assignees

Comments

@ManfredKarrer
Copy link
Contributor

ManfredKarrer commented Oct 2, 2018

As visible in screenshot the "Check if offer is available..." text is not layouted correctly and the spinner is missing. That happens only if there is no amount range, so the next button get skipped and we show directly the funding screen.

screen shot 2018-10-01 at 21 55 16

When the BTC amount can be set by the user then the layout is correct and the spinner is there.

screen shot 2018-10-01 at 21 57 05

Not sure where the text and spinner should be in the first case. At the current position it is easy to overlook and it is not clear why it is there.

@ManfredKarrer ManfredKarrer changed the title Status text at take offer screen not correctly positioned Take offer screen layout issue Oct 2, 2018
@ManfredKarrer ManfredKarrer removed their assignment Oct 13, 2018
@devinbileck
Copy link
Member

devinbileck commented Oct 14, 2018

@ManfredKarrer @ripcurlx As far as I can tell, the "Check if offer is available" text is only applicable if there is an amount range for which the taker needs to specify how much they want to buy. If so, I believe I have a fix to just hide it.

devinbileck added a commit to devinbileck/bisq that referenced this issue Oct 14, 2018
When taking an offer that does not specify an amount range, hide
the "Check if offer is available" text.

Fixes bisq-network#1742
@devinbileck
Copy link
Member

Upon further checking, looks like it is already supposed to be hiding it but isn’t. I will keep digging.

@devinbileck
Copy link
Member

I have opened PR #1775.

It was explicitly hiding the busy animation, but not the label. So my fix also hides the label.

However, I saw isOfferAvailableSubscription is supposed to be hiding it if the offer is available. And after debugging, I saw the offer state was unknown when taking any offer without a range. So that explains why the label was not being hidden. But I am a starting to dive deeper and getting a little lost trying to understand if that is to be expected (unknown state). Plus it is late and time for bed. Any clarification would be appreciated.

@ManfredKarrer
Copy link
Contributor Author

It is a bit tricky.
If there is an offer with range we show the edit amount screen with a next button and then the animation and label text for checking if the peer is online is all fine as the funding screen is not visible before the user clicks the next button and that is only possible after the offer availibility check has succeeded (the label and spinner animation is hidden).
If there is only a fixed amount (offer min amount = offer amount) then we dont need to make that extra step for the user and we show directly the funding screen as the user cannot edit anything in the amount screen. And here the bug got introduced that the layout for the offer availibility check is not updated. In dev mode with regtest you usually dont see that as the offer availibility check gets executed very fast. with real tor connections that can take seconds or even 1 min (timeout).

I am not 100% sure whats the best solution here is, but it should be clearly visible to the user as the offer availibility check is important, so that the user does not fund his wallet from an external wallet before that is successful, otherwise it can be that the offer is not available (either peer is offline or someone else took the offer in the meantime) and the user has made a tx to his bisq wallet from an external wallet. if he used the internal wallet transfer the damage is lower as no tx is created in that case, but still would confuse usability.

@devinbileck
Copy link
Member

Thanks. Makes sense. I will see what I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants