-
Notifications
You must be signed in to change notification settings - Fork 54
Feature/1708 affiliate link owner info show #2026
Conversation
…ure/1708-affiliate-link-owner-info-show
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @maria-vslvn ,
some issues:
-
It would be nice to hide the field when a user is not connected to the Mainnet, as referral data is available only there
-
It would be great to hide the field if a user has not traded before
-
No need to show this link to a user with trades, and these trades were done without a referral link
-
Referral link can't be the connected address. The value is the address that is uploaded to ipfs when a user follows a referral link
-
Seems that the fields are not placed in 1 horizontal line on the form
-
Mobile and tablet views are changed in this PR. It would be great to leave them as they are currently deployed in Prod
@maria-vslvn you should pick the referral address by using the hook And btw there's a typo in |
…ure/1708-affiliate-link-owner-info-show
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, otherwise looking good to me.
Would just make sure Elena's comments are addressed
Hi @maria-vslvn , cases 1-4 from the comment above #2026 (review) are still not fixed. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still do not indicate under which link the 1st trade was done.
@ramirotw @alfetopito Which is the endpoint needed to retrieve the referrer user? |
I believe the way we could do that is:
does that makes sense @alfetopito or we have a simpler and better way to do it? |
Yeah, that sounds about right. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was sure something like that had already been implemented for the affiliate program, before we upload it to IPFS, but looking at the code right now I couldn't find it. Did we just talk about it but did not actually implement it? Anyway, I think this would make sense for both but could be a follow up PR |
@alfetopito You mean this ticket? #1819. @elena-zh shared it to me but I was unsure if it was the same case. |
…-affiliate-link-owner-info-show
@matextrem I've raised this internally with the backend team to check for an alternative solution. Will let you know when I get an answer |
@elena-zh @alfetopito updated! |
I have read the CLA Document and I hereby sign the CLA |
recheckcla |
Hey @matextrem , seems that the last your commit was not properly merged as I still can't see a referree for an account with trades in the Prod server @alongoni , WDYT about left-aligning all these elements in a mobile view like we do for the tablet view? Also, it would be nice to make have a common style: currently, icons, fields' labels have different sizes, colors, etc. |
const [stgResponse, prodResponse] = response | ||
|
||
if (!response.length) { | ||
const [stgErrResponse, prodErrResponse] = results as PromiseFulfilledResult<any>[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment for both fns: there seems to be a lot of any
s. Don't we know the types returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a problem with Promise.allSettled
and typing (https://stackoverflow.com/a/67533949) . I tried the solution suggested there but I was not able to make it work with our current eslint
configuration so I have to type those cases with any
Regarding styles: https://www.figma.com/file/bcoZf5RIgVKDItdOaTZq18/GP-Swap?node-id=3581%3A697 Not sure yet about left-aligning all elements (Referred by + Last updated + View all orders). I'll check it in a prototype on figma. |
Hey @matextrem , great changes! The last thing that I have noticed today is that the account referee info is not auto-removed when switch to another account without trades/referral code. See the video: https://watch.screencastify.com/v/xHXmzYwUEKS80xTjMmWU Could you please fix this? |
…-affiliate-link-owner-info-show
@elena-zh Updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now!
As was discussed in the last sync meeting, we decided to not proceed with this feature. Thanks for the effort everyone, but unfortunately it won't go live. |
Summary
<<if there's an issue>>Fixes #1708
add and modify the affiliate link owner info display ate the profile page
To Test
Profile
Checkout the new display and updates