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

feat: Update network select #385

Merged
merged 8 commits into from
Oct 9, 2023
Merged

Conversation

Lykhoyda
Copy link
Collaborator

@Lykhoyda Lykhoyda commented Oct 5, 2023

Closes #264

  • Updated the network selection for the desktop
  • Updated the network selection for mobile
  • Mobile menu style updates
  • fix "watch account" or "connect" for mobile devices
  • fix loader appearance for MultiProxy selection when there is no proxy

Copy link
Member

@asnaith asnaith left a 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.


  1. The network selector is less round and has a bit more space than the version on the issue / in figma. For example:
CleanShot 2023-10-05 at 15 55 07@2x
  1. On the mobile menu we are still including the network name and the connect button and menu items are centered and not right aligned.
CleanShot 2023-10-05 at 15 43 14@2x
  1. I noticed there's some gaps in the corners of the menu border:
CleanShot 2023-10-05 at 15 23 26@2x
  1. 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


  1. Is the network selection supposed to look like this on mobile?
CleanShot 2023-10-05 at 16 02 22@2x
  1. 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
CleanShot 2023-10-05 at 16 08 58@2x

packages/ui/src/components/select/NetworkSelection.tsx Outdated Show resolved Hide resolved
@Tbaut
Copy link
Collaborator

Tbaut commented Oct 6, 2023

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.

@Lykhoyda
Copy link
Collaborator Author

Lykhoyda commented Oct 6, 2023

Hey Anton, I noticed a couple of things here and also need a bit of clarification on a few things.

  1. The network selector is less round and has a bit more space than the version on the issue / in figma. For example:
CleanShot 2023-10-05 at 15 55 07@2x 2. On the mobile menu we are still including the network name and the connect button and menu items are centered and not right aligned. CleanShot 2023-10-05 at 15 43 14@2x 3. I noticed there's some gaps in the corners of the menu border: CleanShot 2023-10-05 at 15 23 26@2x 4. 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

  1. Is the network selection supposed to look like this on mobile?
CleanShot 2023-10-05 at 16 02 22@2x 6. 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 CleanShot 2023-10-05 at 16 08 58@2x

@asnaith Thank you for the review!

  1. The reason why the selector is not rounded is that the changes should be reflected in the other components as well. Because if we round only the network selector it would look strange. I think it's a recent change in the design to have more rounded components. So it can be changed in a different story as it will touch other components. For consistency, I would keep a similar design for the current PR.
Screenshot 2023-10-06 at 11 02 34
  1. For the mobile menu I keep the name as we don't have a lack of space compared to the desktop version. It's also helpful to distinguish when the icon is the same but has different networks (Assethub-Dot and Assethub-Ksm). IMO it looks better when the items are centered, as your eyes are focused more on the middle of the screen and the text looks more readable. In the newer version of the mobile design the network selector and proxy are not part of the menu, so it will be changed anyway :)

  2. Good catch!

fix "watch account" or "connect" for mobile devices
Screenshot 2023-10-06 at 11 17 54


"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".

Screenshot 2023-10-06 at 11 19 28
  1. So far we don't have a mobile design. I will make it a little bit bigger, but yeah it's how it looks now.

Copy link
Collaborator

@Tbaut Tbaut left a 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.

packages/ui/src/components/select/MultiProxySelection.tsx Outdated Show resolved Hide resolved
packages/ui/src/constants.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Tbaut Tbaut left a 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.

Copy link
Member

@asnaith asnaith left a 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 :)

@Lykhoyda Lykhoyda merged commit eac29c0 into main Oct 9, 2023
6 checks passed
@Lykhoyda Lykhoyda deleted the lykhoyda/update_network_selector_view branch October 9, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework the network dropdown
3 participants