-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: Update network select #385
Conversation
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 Anton, I noticed a couple of things here and also need a bit of clarification on a few things.
- The network selector is less round and has a bit more space than the version on the issue / in figma. For example:
- On the mobile menu we are still including the network name and the connect button and menu items are centered and not right aligned.
- I noticed there's some gaps in the corners of the menu border:
- It was not obvious to me what was fixed with regard to these two points below, could you please advise me on the nature of the change so that I can validate? 🙏
fix "watch account" or "connect" for mobile devices
fix loader appearance for MultiProxy selection when there is no proxy
- Is the network selection supposed to look like this on mobile?
- Lastly (and this could be out of the scope of this ticket) I feel like we should probably use a more distinct colour for the connect button now that we no longer have a blue background, what do you think? cc @Tbaut
For 6. I'd say I've no strong opinion. We could have it primary. But this doesn't have to be tackled in this PR since it's not par of it. I'll give this PR a go after the comments from Andrew are addressed. |
@asnaith Thank you for the review!
fix "watch account" or "connect" for mobile devices "fix loader appearance for MultiProxy selection when there is no proxy" When you switch the network and have no multisig we show the loader and it disappears after checking that there is no multisig. It causes the UI to unnecessarily "jump".
|
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.
Some comments on one decision regarding the loader. For the rest, I don't have strong opinions regarding what was discussed above with Andrew.
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.
Looking good from my side. I think the logo could be bigger in the network menu see my comments below. I opened an issue for the jumping annoyance #387.
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 @Lykhoyda Thank you for all those confirmations and clarifications. I just wanted to make sure what I noticed was intentional / just how it is for now.
I’ll create a separate issue for changing the connect button color.
Looks like we’re all good to go with this then :)
Closes #264