feat: Relates to #264. SUI-based drop-down menu with Backup Account and Add Tokens by ltfschoen · Pull Request #356 · openethereum/fether

ltfschoen · 2019-01-13T02:03:31Z

Usage: On the "Account List" page, click and account to be taken to its "Account" page.
Then click the icon with the three dots to open the menu. Or click anywhere else to hide it.
Replaces PR feat: Relates to #264. Add drop-down menu with Backup Account and Add Tokens #318.
Previous review comments adopted for the menu:
- Account page menu now based on Semantic UI (SUI) Popup instead of custom job.
  - Reference: https://react.semantic-ui.com/modules/popup/
- Place the modal above the button (this means we cannot use popup arrow)
- Max menu height
- Transparent background around the menu (i.e. not grey like a modal)
Proposed changes to previous discussion:
- Use a SUIButton style (i.e. black border, white background)
  for menu items instead of just ordinary text
- If multiple menu items have short names they will be next to each other
  potentially saving the user from scrolling too much if we end up having many items

Screenshots

How it looks with just the two current menu items:

How it would look if we had lots of menu items:

View of the page with just the "three dots" menu icon in the top right that a user can click on:

Tbaut · 2019-01-13T11:08:54Z

Thanks Luke. I haven't tested it yet. Can we make it so that it looks like on the mockup (#264)? With simple lines instead of buttons.

ltfschoen · 2019-01-13T12:02:00Z

Thanks Luke. I haven't tested it yet. Can we make it so that it looks like on the mockup (#264)? With simple lines instead of buttons.

Sure will do

… button style

ltfschoen · 2019-01-13T12:51:12Z

@Tbaut I've updated it so the menu items are like before but with light grey background colour transition effect applied to the item being hovered over.

Screenshot with two items:

Screenshot with multiple items causing scrollbar to appear:

…button

Tbaut · 2019-01-14T10:08:02Z

Just tested, that looks pretty good! A couple comments:

Can we have this behavior that the menu stays displayed as long as the user doesn't click away? (no autoclose)
Could we use the same styling as the other menus for hovers (black on dark grey). Like

…ts svg

ltfschoen · 2019-01-14T12:22:10Z

Just tested, that looks pretty good! A couple comments:

Can we have this behavior that the menu stays displayed as long as the user doesn't click away? (no autoclose)

Could we use the same styling as the other menus for hovers (black on dark grey). Like

@Tbaut I've pushed commits to address your review comments.

Tbaut · 2019-01-14T13:48:09Z

Misunderstanding regarding the 3 dots sorry. I'm totally fine with them.
I was talking about the styling of the menu items on hover (the list "backup account", "add token"). On mouse hover, instead of having the inverted colors + the animation, to simply have the same styling as what we have on other menus, without animation to keep things consistent e.g

.header-nav a:hover, .header-nav a:link:hover, .header-nav a:visited:hover {
    background: rgba(221, 221, 221, 0.5);
    color: #222;
}

Also regarding the click away, one thing missing, similarly to feat: Relates to #264. Add drop-down menu with Backup Account and Add Tokens #318 (comment) it would be great if the click wouldn't trigger anything (there should be a layer between the token layer and the menu) so that if you click on an token card to discard the menu, you don't end up on the sendTx screen.

…close menu

ltfschoen · 2019-01-14T21:50:27Z

@Tbaut Sorry about that. I've pushed commits to address those comments (i.e. layer under menu to prevent clicking another link upon menu blur click, restore three dot icon and fix styling of menu including hover for consistency)

Tbaut · 2019-01-15T15:42:18Z

I think it works well and looks good 🎉
Given the fact that the big PR (Signer) will be merged soon and they conflict (do not show a specific menu item for Signer account) I suggest that we wait and merge this one right after the big one, is that ok?

ltfschoen · 2019-01-15T15:46:04Z

I think it works well and looks good 🎉

Given the fact that the big PR (Signer) will be merged soon and they conflict (do not show a specific menu item for Signer account) I suggest that we wait and merge this one right after the big one, is that ok?

Yes sure, Signer is priority

ltfschoen · 2019-01-18T04:00:40Z

@amaurymartiny I've pushed changes to address your review comments. I haven't had any success implementing Icon from semantic-ui. I've shared what I've tried but I could do with some help.

amaury1093

This is much cleaner! Will merge after the small nits are fixed.

amaury1093 · 2019-01-18T09:56:55Z

packages/fether-react/src/Tokens/Tokens.js

+    ];
+
+    if (this.isParitySignerAccount() === false) {
+      menuItems = [backupAccountItem, ...menuItems];


You can use unshift() here

amaury1093 · 2019-01-18T09:57:09Z

packages/fether-react/src/Tokens/Tokens.js

+      onClick: () => history.push(`/backup/${address}`)
+    };
+
+    let menuItems = [


see above comment, this can be const

amaury1093 · 2019-01-18T09:58:11Z

packages/fether-ui/src/MenuPopup/MenuPopup.js

+            <div
+              className='popup-screen_item'
+              key={item.name}
+              onClick={() => item.onClick()}


onClick={item.onClick}

packages/fether-react/src/assets/sass/components/_header-nav.scss

axelchalon

works well!

ltfschoen · 2019-01-19T07:16:03Z

@amaurymartiny I've pushed commits addressing your latest comments. We can raise an issue to try and implement SUIIcon instead of using the svg's as you mentioned

amaury1093

Only one tiny small nit. Sometimes these shorthands don't work well, so @ltfschoen i would appreciate if you could quickly test it. If it works, feel free to merge this PR!

packages/fether-ui/src/MenuPopup/MenuPopup.js

Tbaut · 2019-01-22T09:15:29Z

The clickaway behavior seems to have come back again after the changes. I opened an issue: #383

ltfschoen added 4 commits January 13, 2019 12:59

feat: Add SUI-popup based account page menu

5f8adc2

fix: Fix linting error by remove disused variable

57ccf0c

fix: Remove incorrect prop from SUI popup

77f6fb8

refactor: Just use state instead of full constructor syntax

04f2462

ltfschoen added the A0-pleasereview label Jan 13, 2019

ltfschoen mentioned this pull request Jan 13, 2019

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

Closed

review-fix: Change menu items to div with background hover instead of…

dc3b2c2

… button style

fix: Remove side padding from buttons. Add transition and opacity to …

88d27c7

…button

Tbaut added A5-grumble and removed A0-pleasereview labels Jan 14, 2019

ltfschoen added 2 commits January 14, 2019 23:15

review-fix: Replace account menu icon with plus icon. Remove three do…

fcc6e26

…ts svg

review-fix: Disable autoclose for the account page menu

1e302be

ltfschoen added 4 commits January 15, 2019 06:44

review-fix: Restore three dot icon for account page menu

8a2e604

add missing newlines

66bd8de

review-fix: Remove menu item animation. Fix menu item consistent styling

c65547a

review-fix: Add menu underlayer to prevent clicking other links when …

e122b38

…close menu

ltfschoen added A0-pleasereview and removed A5-grumble labels Jan 15, 2019

Tbaut approved these changes Jan 15, 2019

View reviewed changes

Tbaut added the A1-onice label Jan 15, 2019

Tbaut removed the A1-onice label Jan 15, 2019

ltfschoen added 8 commits January 18, 2019 12:37

refactor: Replace with defaultProps

402836f

refactor: Pass down props to SUIPopup using otherProps

e46e1f4

refactor: Rename to onItemClick

bedcce1

fix: Use Menupopup with trigger and without underlay CSS

c72e4b7

refactor: Replace ahref with Clickable

e423b8b

refactor: Replace url with onClick

a9b3ae6

Merge branch 'master' into luke-264-account-screen-menu-alternative

c174247

remove useless part of svg

f2c333f

ltfschoen mentioned this pull request Jan 18, 2019

feat: Relates to #337. Part 1 of 2 - Account view - Change header account design #363

Merged

amaury1093 approved these changes Jan 18, 2019

View reviewed changes

amaury1093 added A8-looksgood A0-pleasereview and removed A5-grumble A8-looksgood labels Jan 18, 2019

axelchalon approved these changes Jan 18, 2019

View reviewed changes

ltfschoen added 4 commits January 19, 2019 18:04

Merge branch 'master' into luke-264-account-screen-menu-alternative

37724a3

review-fix: Change to unshift instead of spread operator

42804a1

review-fix: Change to const

479f2b5

review-fix: Remove parenths from onClick

be4da5d

amaury1093 approved these changes Jan 19, 2019

View reviewed changes

packages/fether-ui/src/MenuPopup/MenuPopup.js Outdated Show resolved Hide resolved

amaury1093 added A8-looksgood and removed A0-pleasereview labels Jan 19, 2019

review-fix: Remove anon function and just call onClick

9333ccd

ltfschoen merged commit d67f912 into master Jan 19, 2019

ltfschoen deleted the luke-264-account-screen-menu-alternative branch January 19, 2019 13:46

Tbaut mentioned this pull request Jan 22, 2019

Prevent clicking on any link or button when a menu is open #383

Closed

ltfschoen mentioned this pull request Jan 22, 2019

fix: Fixes #383. Add an underlay to absorb a blur click away from the menu so nothing else in the UI clicked upon menu blur #384

Merged

Conversation

ltfschoen commented Jan 13, 2019

Uh oh!

Tbaut commented Jan 13, 2019

Uh oh!

ltfschoen commented Jan 13, 2019

Uh oh!

ltfschoen commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tbaut commented Jan 14, 2019

Uh oh!

ltfschoen commented Jan 14, 2019

Uh oh!

Tbaut commented Jan 14, 2019

Uh oh!

ltfschoen commented Jan 14, 2019

Uh oh!

Tbaut commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltfschoen commented Jan 15, 2019

Uh oh!

ltfschoen commented Jan 18, 2019

Uh oh!

amaury1093 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaury1093 Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

amaury1093 Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

amaury1093 Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axelchalon left a comment

Choose a reason for hiding this comment

Uh oh!

ltfschoen commented Jan 19, 2019

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tbaut commented Jan 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ltfschoen commented Jan 13, 2019 •

edited

Loading

Tbaut commented Jan 15, 2019 •

edited

Loading

amaury1093 left a comment •

edited

Loading