Giveth / giveth-dapps-v2

This project is the aggregation of GIVeconomy and Giveth.io DApps in a single repo
https://staging.giveth.io
GNU General Public License v3.0
61 stars 34 forks source link

Show token balances in drop down token list #3002

Closed divine-comedian closed 1 month ago

divine-comedian commented 1 year ago

We should update the token list drop down menu on the donate page for a project

image

image

Feature checklist

Figma

https://www.figma.com/file/d3ciTS36toS9mra0RAPMe2/Giveth-Design-System-v0.1?type=design&node-id=5189-12966&mode=design&t=a8nAqx77zJyXDFLx-4

Current flow for handling custom tokens

https://github.com/Giveth/giveth-dapps-v2/assets/67759413/d15bc8a7-ee5a-483d-b5e6-957ec2bcfb4c

divine-comedian commented 1 year ago

@Tosinolawale - want to take a look at this one?

mosaeedi commented 11 months ago

We added the token balance and also made some other improvements!

Screenshot 2023-10-18 at 7 06 09 PM

We are redesigning the whole donation page by adding the recurring donation. This new modal is totally aligned with the new design, which is coming soon!

Design asset

Link to Figma

mosaeedi commented 11 months ago

@divine-comedian Design is done. Please take a look and let me know WDYT!

divine-comedian commented 11 months ago

Thanks @mosaeedi - just left a few comments - otherwise lookin' good!

mosaeedi commented 11 months ago

Thanks @divine-comedian . I've shared my comments as well. If we are all okay, we can hand it over to the devs.

RamRamez commented 9 months ago

@mosaeedi I think the Figma link is outdated. Can you please share the new link?

mosaeedi commented 9 months ago

Sorry, it was moved to another file.

New Figma link

RamRamez commented 9 months ago

@MohammadPCh has already developed this in "superfluid-develop-branch". I think we can close this issue.

divine-comedian commented 9 months ago

@MohammadPCh has already developed this in "superfluid-develop-branch". I think we can close this issue. @RamRamez

Do we fetch all the balances in that branch or just the 5 tokens that are superfluid eligible?

Is there anything else we need to build to accommodate using this modal for single donations?

MoeNick commented 8 months ago

I guess it's p2, I put the priority and I unassigned @RamRamez for now, until we start working on that

divine-comedian commented 6 months ago

Looks like it is added on the recurring donation modal but not on the one-time donation modal - we should make sure they are consistent. @MoeNick can we put this one back on our radar?

https://github.com/Giveth/giveth-dapps-v2/assets/67759413/d3d57896-ac4d-4f98-a8b7-84734766686d

MoeNick commented 6 months ago

Sure @jainkrati Would you please assign it back to devs? It's still p2.

jainkrati commented 5 months ago

@MohammadPCh pls take this up and move to QA if already fixed

MohammadPCh commented 2 months ago

Hey @divine-comedian, do we still support using custom tokens?

MohammadPCh commented 2 months ago

@divine-comedian how about showing gemini modal for donating GIV to givingBlock projects? do we need it or we can remove them?

divine-comedian commented 2 months ago

@divine-comedian how about showing gemini modal for donating GIV to givingBlock projects? do we need it or we can remove them?

Soon we will remove giving block projects to be replaced with endaoment projects, so not worth the effort at this point

Hey @divine-comedian, do we still support using custom tokens?

I don't think we figured out custom token support https://github.com/Giveth/impact-graph/issues/1226#issuecomment-2156805348 but for now we don't need to handle showing balances of custom tokens in the drop down token list

divine-comedian commented 2 months ago

Thanks @divine-comedian so I don't implement it since we need the UI to support custom tokens on the select token modal.

Cherik and I had a short convo - we can start this work and handle the current way we support custom tokens included in this feature as well.

MohammadPCh commented 2 months ago

@maryjaf it's ready to test. We need to check it together since there are many edge cases, and maybe I missed some of them.

MohammadPCh commented 2 months ago

@divine-comedian @maryjaf I fetch the balance of only the visible tokens to reduce the number of requests. Since we have around 140 tokens on mainnet, sending 140 requests might block the user or hit the rate limit. This optimization might limit our ability to sort tokens by balance or indicate if the user has no balance in any token.

divine-comedian commented 2 months ago

@divine-comedian @maryjaf I fetch the balance of only the visible tokens to reduce the number of requests. Since we have around 140 tokens on mainnet, sending 140 requests might block the user or hit the rate limit. This optimization might limit our ability to sort tokens by balance or indicate if the user has no balance in any token.

hmmm.... do we know how other DEXes might handle this feature? Sorting by balance is a very nice feature to have for this.

Also related - looks like when I am staging and have not selected a token yet there is an available balance that is shown...

if I haven't selected a token yet how come it shows a token balance! it looks by default to check the balance of the native token. I would be okay if the native token is always selected by default on page load in the drop down menu, but is that the intention? image

maryjaf commented 2 months ago

I see this changes are on staging env but the status of this issue is "inprogress" Is it ready for testing or not ? Since we have a release in upcoming days if this issue won't be ready it should be reverted

divine-comedian commented 2 months ago

@kkatusic - If you're able to jump in here I will clarify what we need for the release:

fix bug related to showing native token balance when no token is selected.

let's leave out the sorting token balance requirements and work on it after the new version release.

kkatusic commented 2 months ago

@divine-comedian, I fixed that, please check it here

divine-comedian commented 2 months ago

@maryjaf - can you please test this one

lovelgeorge99 commented 2 months ago

@divine-comedian i fixed the sorting based on highest balance here https://github.com/Giveth/giveth-dapps-v2/pull/4416 can you please check

maryjaf commented 2 months ago

Since this issue needs to be tested before release and I'm working on the qf issue now, it would be great if @LatifatAbdullahi has a chance to look into this issue, wdyt? @divine-comedian

LatifatAbdullahi commented 2 months ago

Test Update

@divine-comedian

0

image

kkatusic commented 2 months ago

@LatifatAbdullahi, cc @divine-comedian, please check balance sorting on this PR: here

LatifatAbdullahi commented 2 months ago

@divine-comedian @kkatusic

Test Update

The list should then be sorted to show the highest balances at the top of the list - Pass

image

image

kkatusic commented 2 months ago

@LatifatAbdullahi please when you get time can you test this issue task:

Link for vercel temp link is here

LatifatAbdullahi commented 1 month ago

@kkatusic @MohammadPCh Test Update

Users can click a toggle that hides tokens with zero balances - Test Pass

Before Clicking the check box

image

After clicking the check box

image

kkatusic commented 1 month ago

@LatifatAbdullahi just to notice here, @MohammadPCh resolved this issue, me and @lovelgeorge99 just fix 2 twings.

But thanks @LatifatAbdullahi to testing all options ;)

LatifatAbdullahi commented 1 month ago

Alright, noted, thanks

divine-comedian commented 1 month ago

@kkatusic - we need to make sure we are testing this on staging and not on feature branches - can you please confirm when this is on staging so it can be tested properly.

kkatusic commented 1 month ago

@kkatusic - we need to make sure we are testing this on staging and not on feature branches - can you please confirm when this is on staging so it can be tested properly.

sorry @divine-comedian , I thought this is some fast fix for this to be published on Friday, that's why I ask @LatifatAbdullahi to test on feature branch. And also when I close or someone else close pull request this issue automatically, that's why we left it on feature branch.

kkatusic commented 1 month ago

@LatifatAbdullahi these two fix, about balance order and 0 balance have been merged to staging, when you get time please test, thx.

LatifatAbdullahi commented 1 month ago

@kkatusic @divine-comedian

Test Update on Staging - Pass

Sorting tokens from highest in quantity to lowest - Pass

Before checking the box to hide tokens with zero balance

image

After checking the box to hide tokens with zero balance

image