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

add mobile menu #98

Merged
merged 12 commits into from
May 3, 2023
Merged

add mobile menu #98

merged 12 commits into from
May 3, 2023

Conversation

Lykhoyda
Copy link
Collaborator

@Lykhoyda Lykhoyda commented Apr 20, 2023

Closes #8

@Lykhoyda Lykhoyda requested a review from Tbaut April 20, 2023 16:13
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.

Moving the .gitignore made it so that we're not ignoring the right files any more, can you please check it and commit again to remove the cached files

squid/.gitignore Show resolved Hide resolved
squid/.vscode/settings.json Show resolved Hide resolved
ui/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
ui/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
ui/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
ui/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
ui/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
ui/src/components/Header/index.tsx Outdated Show resolved Hide resolved
ui/src/components/Header/index.tsx Outdated Show resolved Hide resolved
ui/src/components/Header/index.tsx Outdated Show resolved Hide resolved
ui/src/components/layout/Main.tsx Outdated Show resolved Hide resolved
ui/src/components/layout/Main.tsx Outdated Show resolved Hide resolved
@Tbaut
Copy link
Collaborator

Tbaut commented Apr 20, 2023

Since it's a draft, but you requested my review, I'm not sure how much I should review the actual functionality. Sorry if this particular comment isn't helpful and you knew about it:
The issue #8 I wrote isn't super precise, but I meant specifically that the address selection was what was the most mobile unfriendly. This problem isn't solved:

image

Also I think we should have the menu drawer

  • not visible/actionable when on a large enough screen since it adds a click and hides the menu entries. Said differently, the menu should stay the same as it is right now on destop, and the mobile menu should be visible only on smaller screen.
  • it should not push the content, it should be above it ideally, otherwise the whole content jumps all the time

@Lykhoyda Lykhoyda force-pushed the lykhoyda/mobile_menu branch from 2c3950f to 2f4904a Compare April 24, 2023 14:55
@Lykhoyda Lykhoyda marked this pull request as ready for review April 24, 2023 14:58
@Lykhoyda Lykhoyda requested a review from Tbaut April 24, 2023 14:59
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.

Super nice, thank you so much for this. I left a couple comments, but we're super close to be golden :)

ui/src/components/Header/index.tsx Show resolved Hide resolved
ui/src/components/Header/index.tsx Outdated Show resolved Hide resolved
))}
</BoxStyled>
)}
<MultiProxySelection/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add the MultiProxySelection and the NetworkSelection to the mobile menu?
The Kusama deployment is imminent and while the mobile view is better without the menu, I think it won't be manageable with both the network selector and the account selector in the top bar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it for sure, but as a design update are pretty close we probably could wait until the final version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the design update will really take a while to land, and start, so I think it's a low effort high reward thing to do.

ui/src/components/layout/Main.tsx Show resolved Hide resolved
ui/src/components/layout/Main.tsx Show resolved Hide resolved
@Tbaut Tbaut requested a review from asnaith April 25, 2023 21:34
@Tbaut
Copy link
Collaborator

Tbaut commented Apr 25, 2023

BTW @asnaith, and also useful to you @Lykhoyda if you want to take a look at a PR and don't want to bother building the branch, you can go to the deployment page, where every commit is deployed. It's available from the main repo page as well:
image

And click on the view deployment. This will bring you to the deployment of the related commit. I noticed the commit hash don't match though, you should hover it, to see if it's the PR you're looking for.

@asnaith
Copy link
Member

asnaith commented Apr 26, 2023

Thanks for the tip about deployments @Tbaut 👍

@Lykhoyda I tested this PR out within mobile on both iOS and Android within Nova Wallet. The menu is functioning fine for me but I did notice a couple of other issues whilst viewing on mobile. I've logged them as separate issues (#105 and #106).

@Tbaut Tbaut self-requested a review May 3, 2023 14:46
@Tbaut Tbaut merged commit 494ec74 into main May 3, 2023
@Tbaut Tbaut deleted the lykhoyda/mobile_menu branch May 3, 2023 14:46
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.

Mobile friendly menu
3 participants