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

Menu refactoring using popup #227

Merged
merged 7 commits into from
Mar 29, 2019
Merged

Menu refactoring using popup #227

merged 7 commits into from
Mar 29, 2019

Conversation

Tbaut
Copy link
Contributor

@Tbaut Tbaut commented Feb 26, 2019

screenshots:

@Tbaut
Copy link
Contributor Author

Tbaut commented Mar 1, 2019

@ltfschoen if you have time could you give it a shot with an iphone to make sure things compile properly, I could only test on android.

@maciejhirsz
Copy link
Contributor

maciejhirsz commented Mar 1, 2019

I'm digging the UI. You're still working on this (as per the edit in the top comment)?

@Tbaut
Copy link
Contributor Author

Tbaut commented Mar 1, 2019

Nop I'm done. Removing the edit as it caused confusion

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

LGTM, just super pedantic OCD formatting nitpicks :P.

src/components/PopupMenu.js Outdated Show resolved Hide resolved
src/screens/AccountList.js Outdated Show resolved Hide resolved
@Tbaut
Copy link
Contributor Author

Tbaut commented Mar 1, 2019

👍 comments addressed. I'd like to get a confirmation from @ltfschoen before merging

@ltfschoen
Copy link
Contributor

@ltfschoen if you have time could you give it a shot with an iphone to make sure things compile properly, I could only test on android.

Yes, I just got my hands on an iPhone so will look at it asap

@lexfrl
Copy link
Contributor

lexfrl commented Mar 2, 2019

There is no such a menus on iOS. So I predict user's dissatisfaction if such a menu will appear in iOS app too (I am an iPhone user). As an alternative and in order to reach coherence between platforms I'd recommend to use the following menu:

1 wspl_j5wdpjbrml69pfm0g

The better idea - to is avoid of using the "popup" menu at all and use a separate screen. I don't see any reason to not use the current dedicated screen we have.

In a conclusion: the idea to move the button to top - it's a good idea. The idea to make it as a popup menu - it's a wrong idea. It's better to just navigate user (on tap) to the following screen:

image 2019-03-02 18 15 07

This way we can successfully avoid the unnecessary addition of the not native UI element.

Reference: https://medium.com/@chunchuanlin/android-vs-ios-compare-20-ui-components-patterns-part-1-ad33c2418b45

@lexfrl
Copy link
Contributor

lexfrl commented Mar 2, 2019

@Tbaut
Copy link
Contributor Author

Tbaut commented Mar 3, 2019

I'm sorry but I disagree. Also I believe that this PR is not the right place to discuss design decisions. Although this implementation is surely not perfect, it streamlines processes and makes the app easier to navigate. Things are not set in stone, we can change them later on (and discuss them in an issue).

@lexfrl
Copy link
Contributor

lexfrl commented Mar 4, 2019

Also I believe that this PR is not the right place to discuss design decisions.

where is the right place then?

Although this implementation is surely not perfect, it streamlines processes and makes the app easier to navigate.

It's against guidelines. It's frustrating for (iOS at least) users.

@maciejhirsz
Copy link
Contributor

Apple guidelines are just that, guidelines, it's not a crime to break them. Given that we are making a cross-platform app, not adhering to guidelines of the underlying OS is, maybe not ideal, but totally fine.

UI or UX issues that would be show stoppers here are things like:

  • Something is unclear or confusing.
  • Something takes more/unnecessary amount of steps to do producing friction.

On both of these points I think the UI changes are improvements. The PR is accepted, it will be merged as soon as we solve the (recurring) issues with ring crate compiling that are unrelated to this PR specifically.

@novasamatech novasamatech locked as resolved and limited conversation to collaborators Mar 5, 2019
@maciejhirsz maciejhirsz merged commit 228b11d into master Mar 29, 2019
@maciejhirsz maciejhirsz deleted the tbaut-popup-menus branch March 29, 2019 13:53
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.

Enhance menus
4 participants