Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

feat: Relates to #264. Add drop-down menu with Backup Account and Add Tokens #318

Closed
wants to merge 14 commits into from

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Dec 20, 2018

Implementation of this wireframe #264 (comment).

Will allow other items to be listed including View Transactions in subsequent PR to address #98

Screenshot:

screen shot 2018-12-20 at 6 20 34 pm

Usage: User clicks the icon and the pop-up menu appears. If they click the icon again the popup modal closes. If they click one of the menu items they'll be navigated to the relevant page and the popup menu is no longer shown.

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 20, 2018

  • One thing I found weird while trying it is that it doesn't close when you click away. I think it's something natural in menus.
    (If this is tricky to implement, have a look at https://react.semantic-ui.com/modules/dropdown as we have semantic UI as a dependency already)
  • Regarding the design, it feels a little small and hard to read. What about having the background as white as the card's background, it looks good as long as there is a dropped shadow (see below)
  • We shouldn't be afraid to let it take space, can we let the menu overflow on the icon like the Mockup (this is for the account list but you get the idea):
    image

Edit: removed the close button

@ltfschoen
Copy link
Contributor Author

@Tbaut I've addressed your latest review comments. It has a bug that I've mentioned below, but want to get your feedback before I spend more time on it.

Usage: It now shows three-dots icon when not menu not open, then when click three-dots icon it displays cross icon overlay along with the menu. The menu now has drop-shadow that darkens upon hover. If you hover over the menu items the font colour lightens. I've added a spacer between the menu items. I've made each menu item a div instead of a href as it's annoying having to precisely click on the link text instead of anywhere on the menu item row. If you click the cross icon it closes the menu. Likewise if you click anywhere else other than the menu area it closes the menu.

BUG: Whilst the user can also now close the menu by clicking anywhere outside the menu area to close the menu (i.e. blur), doing so correctly closes the menu, however it still shows the cross icon instead of switching back to the triple-dot icon. So to show the menu again the user has to first click the cross icon so it turns back into a triple-dot, and then click the triple-dot to open the menu again.
If the user closes the menu by clicking the cross icon there is no bug.

Screenshot:

screen shot 2018-12-20 at 11 02 26 pm

@ltfschoen
Copy link
Contributor Author

Additionally since we're moving to Semantic UI as mentioned by @amaurymartiny we could try replacing the icons with https://react.semantic-ui.com/elements/icon/ (i.e. close and ellipsis horizontal)

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 21, 2018

I'm sorry to slightly change my mind, I've had a look at a lot of apps out there, most such as Whatsapp do not show such a close button. The overall look and feel from the dropdown is good otherwise. I'd also drop the border bottom from the spacer, it lets the menu feel lighter.

Regarding the close on "click away" I think there should be a layer (with a z-index on the the wrapper?) above any window element so that you can actually click anywhere, even on an account card, and all this would do it close the menu. Example on Github, click the edit button, and then click on a link while the menu is open. What happens is that the menu gets closed, but the link you clicked didn't redirect you.

image

@amaury1093
Copy link
Collaborator

amaury1093 commented Dec 21, 2018

SUI's Popup with on="click" does exactly the above behavior (reverify that the invisible layer does indeed catch the click).

… show/hide menu without state

* Use CSS to show/hide menu instead of using React state. Menu and overlay hidden by default

* Overlay is grey to make it clear the user's Call to Action (CTA) is to select a menu item

* Use can click overlay to close menu

* Remove close icon

* Remove spacer line
@ltfschoen
Copy link
Contributor Author

ltfschoen commented Dec 25, 2018

@Tbaut I've addressed your review comments, which fixed the bug too. Thanks for the overlay suggestion, it hadn't come to mind!

I've updated it so you click the three-dot icon to open the menu, which also displays a grey/transparent overlay (similar to a modal) that the user can click to close the menu.
It displays the menu a bit lower similar to the way Github does it as you showed.
I've also added some minor animations for when the user moves their mouse between the menu and the overlay.

screen shot 2018-12-25 at 11 36 36 am

I've set it up so it auto scrolls incase we add more items than fit in the page in future. i.e.

screen shot 2018-12-25 at 11 35 51 am

@amaurymartiny Happy to try SUI as any alternative if necessary.

@ltfschoen
Copy link
Contributor Author

@Tbaut I've created a component called Modal in fether-ui and used that for the menu so we can reuse the code.

@amaurymartiny are you ok with this? Should I take it further and use a CSS-in-JS library to move the styles into the component?

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 27, 2018

Thanks a lot @ltfschoen
from the screenshots (haven't tested yet), I'd like to keep the background transparent because it feels too much like a modal otherwise and not enough like a menu. (to keep it like github or whatsapp)

@ltfschoen
Copy link
Contributor Author

@Tbaut i've uploaded a couple of videos showing how it appears, please confirm which one you'd like:

  1. Modal-like menu experience with gray-coloured overlay - https://drive.google.com/open?id=1NliuRLuVkU_8KKXjdvZ0Man16mfbEsTx
  2. Github-like menu experience with transparent overlay - https://drive.google.com/open?id=1JPM6MGq0WH3zt6lJWEzAVTBWj-f4RmtT
  3. open to any other requests or suggestions :)

i'll update the PR after receiving confirmation of which option you prefer.

FYI, i went with the modal-like appearance because i thought it channelled the users focus on the menu (it changes from a lighter to a darker gray and vice versa as they move over the list of menu items). i personally find that a modal-like grayish overlay makes it more obvious that i can close the menu by clicking away from the menu popup

@amaury1093
Copy link
Collaborator

I'm good with both videos.

@ltfschoen Ok to create a component in fether-ui. But can you try to use SUI's components as a base? Modal or Popup. The idea of #158 is to use SUI whenever possible, so that we have the least amount of custom styling possible.

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 2, 2019

I'd stick with a transparent background for menus because it's the regular behavior for apps.
Could we place the modal above the button as before (like the mockup)?
Also instead of changing the font weight on hover, maybe change the background so that the menu height doesn't change ?

Edit: I've checked dozens of apps and web apps including whatsapp, gmail, medium, twitter, google playstore, none of them apply a grey background. This is reserved for big drawer menus (taking the whole page height).

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 8, 2019

Hey @ltfschoen, any roadblock regarding this pr?

@ltfschoen
Copy link
Contributor Author

@Tbaut Next step is to try and build the same but based on using SUI Modal or Popup with styled-components so it builds upon something as generic as possible for reusability (instead of this custom job). So I'd leave the PR status as in-progress

@ltfschoen
Copy link
Contributor Author

Replaced by this PR that builds a solution based on a SUI popup #356

@ltfschoen ltfschoen closed this Jan 13, 2019
@Tbaut Tbaut deleted the luke-264-account-screen-menu branch January 18, 2019 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants