Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Simplify links #2266

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Simplify links #2266

merged 2 commits into from
Jan 24, 2022

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Jan 23, 2022

Summary

This PR simplifies the logic of the buttons in the top part:

image

Hopefully, now the logic should be easier to read, and also how they work should be simpler too. The rules are:

  • We allow to change the account : Only in the default status and confirmed status. Meaning, when we are not claiming or we just sent the tx to the wallet. Additionally, within the investment flow, we don't show the buttons in the last step.
  • We never show the two links: Before we were showing sometimes Change account and sometimes Switch to connected account. Now, only one of those will be visible. If you want to go back and use your connected wallet, normally you will need to first click on "change account", then in "Switch to connected account"

Before

We used to show most of the times the two buttons
image

After

image

image

To Test

  1. Do some full workflows of investing and make sure links are good all the time

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

{allowToChangeAccount && hasActiveAccount ? (
<ButtonSecondary onClick={handleChangeAccount}>Change account</ButtonSecondary>
) : (
!!account &&
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well make this it's on state const to or is this for TS complaining about account?

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

approved based on code logic and app doing what your description claims. not sure if having to click twice now to switch to connect and then change is good but i'm fine with it

@elena-zh
Copy link

elena-zh commented Jan 24, 2022

Hey @anxolin , Changes LGTM!
However, I noticed that account's data is not displayed when I change an account in the connected wallet.
AFAIK, we should show claim table overview for the connected account, but still we show data for the previous account, and in order to see the connected account data we need to press on the 'Change account', then to 'Switch to the connected account.
See the video: https://watch.screencastify.com/v/Ze97bzuzcI8QQszLMzvJ

@anxolin anxolin requested review from a team January 24, 2022 18:05
@alfetopito
Copy link
Contributor

Hey @anxolin , Changes LGTM! However, I noticed that account's data is not displayed when I change an account in the connected wallet. AFAIK, we should show claim table overview for the connected account, but still we show data for the previous account, and in order to see the connected account data we need to press on the 'Change account', then to 'Switch to the connected account. See the video: https://watch.screencastify.com/v/Ze97bzuzcI8QQszLMzvJ

I think that's the expected behaviour.
If you change the connected account we intentionally keep the account being claimed loaded

@anxolin anxolin merged commit 71ecc44 into develop Jan 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants