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

[Claim] check other chains for claimable amts + click to change networks #2313

Merged
merged 16 commits into from
Jan 27, 2022

Conversation

W3stside
Copy link
Contributor

Summary

Shows claims in other chains and allows clicking chain name to change network

Closes #TBD

Add screenshots if applicable. Images are nice :)
What it looks like
image

Can click and change networks:
image

To Test

  1. <> Open the page about
  • <<What to expect?>> Verify it contains about information...
  • Checkbox Style list of things a QA person could verify, i.e.
  • Should display Text Input our storybook
  • Input should not accept Numbers
  1. <> ...

Background

Optional: Give background information for changes you've made, that might be difficult to explain via comments

@W3stside W3stside changed the base branch from develop to release/1.10 January 26, 2022 18:32
@W3stside W3stside changed the title Claim check other chains pClaim] check other chains for claimable amts + click to change networks Jan 26, 2022
@W3stside W3stside changed the title pClaim] check other chains for claimable amts + click to change networks [Claim] check other chains for claimable amts + click to change networks Jan 26, 2022
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I like very much how this idea is implemented, two quick feedbacks:

  1. It's telling me I have a claiming available in GC, but this is where i am and I don't see it.
    image

  2. It doesn't make sense to show you have a claiming in the network you are. I would exclude the current network from the message.

What about:

You ${hasClaim ? ' also' : '' } have available claims on ${otherNetworks}

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

A bit big too review the code. I realised some of the comments from my review are related to the change network hook, which i later realised is a copy pasta, so not your code.

Anyways, maybe this case we can change it, since we didn't do a mod, and we just duplicated the logic to own it.

src/custom/hooks/useChangeNetworks.ts Show resolved Hide resolved
src/custom/hooks/useChangeNetworks.ts Outdated Show resolved Hide resolved
src/custom/hooks/useChangeNetworks.ts Show resolved Hide resolved
src/custom/state/claim/updater.tsx Outdated Show resolved Hide resolved
src/custom/state/claim/updater.tsx Outdated Show resolved Hide resolved
@alfetopito
Copy link
Contributor

Suggestion about UI

The banner is not as "round" as the other styles used on claim UI
Screen Shot 2022-01-26 at 15 54 35

Example
Screen Shot 2022-01-26 at 15 54 50

Also, what about re-using the same banner that tells you the account has claims for this?

Instead of the checkmark, use an exclamation and the same text.

@elena-zh
Copy link

elena-zh commented Jan 27, 2022

In addition to @alfetopito comments above, I'd like to propose change text banner to 'This account has available claims in <>' due to 'You have..' sounds a bit illogical when check available claims on behalf of someone else.
image

@elena-zh
Copy link

Some other issues:

  1. It would be nice to remove link from 'and'
    image

  2. Make banner's text center-aligned?
    image

  3. Remove links from the banner when it is impossible to switch to another network (wallet does not support OR connected using WC):
    image

  4. Maybe we could show this banner outside the Claim area like we do for the affiliate flow?
    image
    image

W3stside added a commit that referenced this pull request Jan 27, 2022
* saving

* fix modals and broken network change

* remove debug code
W3stside added a commit that referenced this pull request Jan 27, 2022
* saving

* fix modals and broken network change

* remove debug code
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

This is a great feature! works very well now.

I agree with all design suggestions. This PR is also big enough, should we merge and do a follow up?

src/custom/pages/Claim/ClaimsOnOtherChainsBanner.tsx Outdated Show resolved Hide resolved
@fairlighteth
Copy link
Contributor

Agree on merging to follow up. My feedback on @elena-zh 's points:

  1. Agreed.
  2. Agreed (can address).
  3. Makes sense, agreed.
  4. I would show it inside the container. In my opinion that's contextually the most relevant position.

@elena-zh
Copy link

In addition to @alfetopito comments above, I'd like to propose change text banner to 'This account has available claims in <>' due to 'You have..' sounds a bit illogical when check available claims on behalf of someone else. image

Will we address this?

@elena-zh
Copy link

I have just retested changes when connected to Gnosis Safe: is it possible to have safe with the same address in all networks?
image

If not, I'd rather not to show this message when connected to Gnosis safe

* saving

* fix modals and broken network change

* remove debug code
* check consumed claims using modded hooks

* number > SupportedChainId
* use NotificationBanner instead of PhishAlert

* styles
@W3stside
Copy link
Contributor Author

I have just retested changes when connected to Gnosis Safe: is it possible to have safe with the same address in all networks? image

If not, I'd rather not to show this message when connected to Gnosis safe

lets reference in another issue please

@elena-zh
Copy link

@W3stside , I reported it in #2333

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.

5 participants