gnosis / cowswap

🐮 CowSwap: First Gnosis Protocol v2 UI
https://gnosis.io
GNU General Public License v3.0
113 stars 55 forks source link

Profile page: Add vCOW swap feature cards #2564

Closed fairlighteth closed 2 years ago

fairlighteth commented 2 years ago

Summary

🡒 Note: This is part of a waterfall PR series 🡐

Desktop

Screen Shot 2022-03-18 at 12 28 19

Tablet/Medium resolutions

Screen Shot 2022-03-18 at 12 28 03

Mobile/small resolutions

Screen Shot 2022-03-18 at 12 27 18

No vCOW card container: Governance card falls in to place:

Screen Shot 2022-03-18 at 15 34 34

Only the COW balance:

Screen Shot 2022-03-18 at 15 35 08

Just to demonstrate the sizing flexibility of the card elements.


Light mode

Screen Shot 2022-03-18 at 15 43 44

Todo

github-actions[bot] commented 2 years ago
fairlighteth commented 2 years ago

Noticed on medium resolution, the COW balance token icon position/margin could be improved. Will handle in a up following PR (cc @elena-zh )

alfetopito commented 2 years ago

Noticed on medium resolution, the COW balance token icon position/margin could be improved. Will handle in a up following PR (cc @elena-zh )

You mean this, right?

Screen Shot 2022-03-18 at 10 51 13
elena-zh commented 2 years ago

Hey @fairlighteth , great changes! And thanks for making me aware of the issue about token icon position/margin as I wanted to report it enhance aligment.jpg

What I'd like to add, is that it would be nice to make alignment of all components more or less the same in a middle-screen devices. Look: maybe center.jpg

Another proposal I'd like to make, is to re-use a component from the bottom affiliate forms to show Vested tokens amount. WDYT? image.png

Just to confirm: if a user does not have (v)COW tokens on the balance/not connected, we will show a regular Profile page without newly added components, right?

I have to notice, that I was not able to test cases when a user does not have vCOW card container, or COW balance only, so I will retest it later. All the rest changes LGTM (tested in Win10+Chrome, FF, Brave, iOS 15.3 +Chrome, Androind 10+Chrome).

fairlighteth commented 2 years ago

Updated the tooltip icons to be dynamically use the right dark/light mode colors. Also updated the (swap) container background color to match the others below:

https://user-images.githubusercontent.com/31534717/159471277-94192a9e-991f-42df-82d2-18519abb8f8d.mov

fairlighteth commented 2 years ago

@elena-zh Made some updates to properly align elements for different viewports:

Screen Shot 2022-03-21 at 17 13 01 Screen Shot 2022-03-21 at 17 12 49
fairlighteth commented 2 years ago

Just to confirm: if a user does not have (v)COW tokens on the balance/not connected, we will show a regular Profile page without newly added components, right?

@elena-zh I think at the very least we show the COW balance + governance banner. I think it's beneficial to show both these to make users more aware of COW/Governance. Perhaps we could even add a button 'Buy COW token' when your balance is 0. Obviously this can only be implemented once vCOW is swappable.

the vCOW / convert card we would not show, unless you have >0 vCOW balance.

alfetopito commented 2 years ago

BTW, latest changes are failing the build

Screen Shot 2022-03-21 at 15 59 08

Just comment out/remove those variables

fairlighteth commented 2 years ago

@alfetopito Right, forgot that it would break the build, fixed now!

elena-zh commented 2 years ago

Hey @fairlighteth , changes LGTM now! The only thing I'd like to mention, that it would be nice to make formatting of these amounts locale dependable image

Also, it would be nice to show full amounts on hover as we do for all amounts across the app image

But I feel that everything I have mention is our of scope of the current task.

Thanks!

fairlighteth commented 2 years ago

@elena-zh Thanks for reviewing again:

The only thing I'd like to mention, that it would be nice to make formatting of these amounts locale dependable Also, it would be nice to show full amounts on hover as we do for all amounts across the app

Yes agreed, let's keep this in mind when the actual balances are connected (now hard coded).

elena-zh commented 2 years ago

@fairlighteth , I have created #2571 task for this

fairlighteth commented 2 years ago

One is, that i think in responsive, is more relevant the COW than vCow so it should be higher

Yes, I can see that. I think I'll add a custom 'order' prop on an upcoming PR for the vCOW container.

The other, is I kind of liked more your original proposal for the background of the vested amount. I agree with making it coherent, but maybe your proposed background was nicer than the one we had for the affiliate.

Agreed. Didn't want to add too many changes on this PR and just make it coherent instead. Will adjust this in an upcoming PR for all elements on this page.