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

Adds a ENS address selector to the Profile page #367

Merged
merged 25 commits into from
Apr 18, 2022

Conversation

ramirotw
Copy link
Contributor

@ramirotw ramirotw commented Apr 4, 2022

Summary

Close #190

This adds a new AddressSelector component to the Profile page with all the ENS addresses the user has. It uses the official ENS subgraph

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

CLA Assistant Lite:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger the CLA Action by commenting recheckcla in this Pull Request

@elena-zh
Copy link

elena-zh commented Apr 5, 2022

Hey @ramirotw , could you please provide me the PR preview link please?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@ramirotw
Copy link
Contributor Author

ramirotw commented Apr 8, 2022

@elena-zh the build is finally fixed.

Btw, the CLA check fails because of the prettier bot that commited Fix code style issues with Prettier

{
  id: 'MDQ6VXNlcjU5MjgzODYy',
  databaseId: 59283862,
  login: 'lint-action'
}

@@ -102,7 +102,7 @@
"eslint-plugin-simple-import-sort": "^7.0.0",
"ethers": "^5.4.6",
"graphql": "^15.5.0",
"graphql-request": "^3.4.0",
"graphql-request": "^4.2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced this change to force the CI script to run the postinstall npm script which runs the yarn graphql:generate script which generates the graphql TS types in src/state/data/generated.ts that is needed to import the EnsNamesQuery type.

@elena-zh
Copy link

Hey @ramirotw !
Nice changes!
Unfortunately, I can't finish testing until the issue cowprotocol/explorer#63 is fixed.

Some tiny notes from the UI side:

  1. Maybe we should not show an 'expandable' dropdown when a connected wallet does not have an ENS name for an account? WDYT?
    disable when not available
  2. Once connected to an account with ENS name, maybe we should show the an affiliate link with ENS name by default instead of the address, as the account info shows ENS name?
    image
  3. Can we align link nicer in a mobile view?
    allign better

Thanks!

@ramirotw
Copy link
Contributor Author

3. Can we align link nicer in a mobile view?
allign better

@alongoni any idea on how we could improve this on mobile?

@ramirotw
Copy link
Contributor Author

  1. Maybe we should not show an 'expandable' dropdown when a connected wallet does not have an ENS name for an account? WDYT?
    disable when not available

done

2. Once connected to an account with ENS name, maybe we should show the an affiliate link with ENS name by default instead of the address, as the account info shows ENS name?
image

I have already done this 0bc1579#diff-091719e9c8faa4dd8e97d8f607ee1f288ba02bb4f6be40589de8de2411e589baL36 but there were a few issues while switching accounts and maintaining state so to keep things simple I removed it. Maybe we can add that feature after this gets merged.

@elena-zh
Copy link

Hey @ramirotw , seems that the last commit has failed, as i do not see any changes relates to the case 1.
As for the case 2, I have created a separate issue for this case #404 .
@alongoni , could you please take a look at the case 3?

@ramirotw
Copy link
Contributor Author

Hey @ramirotw , seems that the last commit has failed, as i do not see any changes relates to the case 1.

@elena-zh aren't you seeing this?

image

@elena-zh
Copy link

Hey @ramirotw , seems that the last commit has failed, as i do not see any changes relates to the case 1.

@elena-zh aren't you seeing this?

image

@ramirotw , I see this.. However, I thought that the fix should disable the dropdown when a user does not have an ENS name, so I thought that there were issues with the last commit.
By disabling the dropdown I mean this:
image

@ramirotw
Copy link
Contributor Author

@elena-zh sorry for the misunderstanding. Now the dropdown should be disabled when there are no ens names

@elena-zh
Copy link

@ramirotw , great, thank you!
But could we please remove this hovered' and 'clicked' effects from the dropdown now?
image

I still see the arrow that should expand the dropdown, so it seems that it is still enabled.

@ramirotw
Copy link
Contributor Author

I still see the arrow that should expand the dropdown, so it seems that it is still enabled.

that is the disabled state of the dropdown, or you mean to remove entirely the dropdown as we do for Rinkeby/GC?

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Awesome! I bet Vitalik would like that =P
Screen Shot 2022-04-12 at 17 21 37

And I do agree with Elena's comment of not showing the dropdown at all when there's a single option (no ENS).
Feels like it gives the false impression it's clickable, even though it's disabled.

src/custom/pages/Profile/AddressSelector.tsx Outdated Show resolved Hide resolved
src/custom/pages/Profile/ens.ts Outdated Show resolved Hide resolved
@alongoni
Copy link
Contributor

alongoni commented Apr 12, 2022

  1. Can we align link nicer in a mobile view?
    allign better

@alongoni any idea on how we could improve this on mobile?

Maybe we can use a shorter version of the address e.g: 0x3d2a...1b21

@elena-zh
Copy link

I still see the arrow that should expand the dropdown, so it seems that it is still enabled.

that is the disabled state of the dropdown, or you mean to remove entirely the dropdown as we do for Rinkeby/GC?

Yes, I would remove it at all when an account does not have a ENS name

@elena-zh
Copy link

  1. Can we align link nicer in a mobile view?
    allign better

@alongoni any idea on how we could improve this on mobile?

Maybe we can use a shorter version of the address e.g: 0x3d2a...1b21

that works for me!

@elena-zh
Copy link

Hey @ramirotw , great changes!
I was also able to check that the trade was recorded to the correct referrer (my previous tests) despite of the fact that the issue cowprotocol/explorer#63 is still here.

However, sometimes I face this issue: when I share a referral link with an ENS name, the app shows me 'invalid' banner for a couple of seconds.
More often I can face it when I paste the link to a new browser/1st time opening the link. Then, the issue becomes not reproducible.
See the video: https://watch.screencastify.com/v/qvteh2AKFxhzxxUwUAMz

Could you please take a look into this issue?

Thanks!

@ramirotw
Copy link
Contributor Author

Hey @ramirotw , great changes! I was also able to check that the trade was recorded to the correct referrer (my previous tests) despite of the fact that the issue cowprotocol/explorer#63 is still here.

However, sometimes I face this issue: when I share a referral link with an ENS name, the app shows me 'invalid' banner for a couple of seconds. More often I can face it when I paste the link to a new browser/1st time opening the link. Then, the issue becomes not reproducible. See the video: https://watch.screencastify.com/v/qvteh2AKFxhzxxUwUAMz

Could you please take a look into this issue?

Thanks!

hey @elena-zh, I fixed both issues. Please run a few affiliate workflows to be sure nothing broke because I did some changes to fix the first invalid state

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @ramirotw , changes look great!

The only issue I can mention is that due to Explorer issue cowprotocol/explorer#63 Cowswap app does not receive a successful transaction pp-up, so a referral URL is not removed from the local storage after a successful trade and remains to be displayed.
But this issue is not related to your changes, so the PR is approved.

Thanks

@ramirotw ramirotw merged commit d983da0 into develop Apr 18, 2022
@ramirotw ramirotw deleted the ramirotw/issue-1682-ens-affiliate branch April 18, 2022 11:27
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 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.

[Affiliate] Use ENS on affiliate link
5 participants