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

[Uni Merge] Style fixes pt. 1 #380

Merged
merged 19 commits into from
Apr 14, 2022
Merged

[Uni Merge] Style fixes pt. 1 #380

merged 19 commits into from
Apr 14, 2022

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Apr 6, 2022

Summary

Closes #339

WIP

Pointing at unimerge but will go after and into develop

@W3stside W3stside requested review from a team April 6, 2022 08:52
@W3stside
Copy link
Contributor Author

W3stside commented Apr 6, 2022

NetworkSelector dropdown only looks strange on non-mobile emulated sizes. this is my mobile view:
Screenshot 2022-04-06 at 11 21 03

@W3stside W3stside mentioned this pull request Apr 6, 2022
24 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@elena-zh
Copy link

elena-zh commented Apr 6, 2022

Hey @W3stside , great job!

However, I can find some issues not fixed:

  1. Still. I see a light boarder for the Connect header button
    connect 1
    white 1
    Compare with the button that is exists on the Prod now:
    no boarder (Prod)
    no shadow
  2. I still see that Networks in the selector are not aligned (icons). Or we should leave it as it is?
    not aligned
  3. Can we align arrow pointing to the recipient field better to be in line with the arrow above?
    align better
  4. Still Network selector exceeds right screen boarder in mobile devices
    image
  5. Still network selector is opened 'on hover' event instead of the 'on-click' as we currently use on Prod. So when I press on the network selector twice, it remains opened until I remove a mouse from it.

Thanks

@fairlighteth
Copy link
Contributor

Something I noticed:

  • A weird delay on desktop, when clicking the ellipsis (...) menu button. Here I move my mouse and immediately click:
Screen.Recording.2022-04-06.at.14.17.14.mov

@fairlighteth
Copy link
Contributor

@elena-zh Personally I'd leave 2) and 3). At least for now for the sake of merging.

@elena-zh
Copy link

elena-zh commented Apr 6, 2022

@fairlighteth , OK! Once we merge this PR, I will create separate issues for these cases (unless we decide to leave it as it is)

@W3stside
Copy link
Contributor Author

W3stside commented Apr 6, 2022

thanks @elena-zh

2 is how uniswap does it so i'm going with the It's just the UNI way ™️
3 is actually an improvement on production 😂 (agree its weird anyhow)

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alfetopito
Copy link
Collaborator

alfetopito commented Apr 13, 2022

Regarding Elena's points 2, 3 and 5:
(edit: removed screenshot as it's related to something else. Now I don't remember what I meant to put here 🤷 )

Meaning, it's intentional, and inherited from Uniswap.
In yet another words, it's a feature not a bug ;)

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.

Thanks for the fixes!

Only note is to apply some of the changes to the mod files rather than originals otherwise they will likely be wiped on next update

src/components/Header/NetworkSelector.tsx Outdated Show resolved Hide resolved
@elena-zh
Copy link

OK then!
But let fix please case 1 and 4: connect to a wallet button still does not look good. Besides, it is not a good UI/UX experience when the network selector exceeds right screen boarder in mobile devices.. =)

Base automatically changed from 2440/uni-merge to develop April 14, 2022 14:57
@W3stside
Copy link
Contributor Author

OK then! But let fix please case 1 and 4: connect to a wallet button still does not look good. Besides, it is not a good UI/UX experience when the network selector exceeds right screen boarder in mobile devices.. =)

but it doesnt for me, only does in emulated chrome
@elena-zh u see this on ur mobile device or on web eumlated?

@fairlighteth
Copy link
Contributor

but it doesnt for me, only does in emulated chrome
@elena-zh u see this on ur mobile device or on web eumlated?

It at least happens on iOS simulator as well:
Screen Shot 2022-04-14 at 16 29 15

But this effect has been removed in our latest release (glowing effect). If it's still present here then we should be careful about not accidentally introducing it again.

@fairlighteth
Copy link
Contributor

Also the extra border that appears in 'Connect a wallet' can be seen on iOS (xCode simulator). I think the issue is two competing background colors (not even padding/margin).

@W3stside
Copy link
Contributor Author

@elena-zh @fairlighteth addressed issues 1 and 4 please post merge review

@W3stside W3stside merged commit c2e3b84 into develop Apr 14, 2022
@W3stside W3stside deleted the uni-merge-style-fixes-1 branch April 14, 2022 19:15
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2022
@elena-zh
Copy link

@W3stside , the button and the network selector's position look good now!
Thanks!

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.

UI issues related to PR 2440/uni merge
4 participants