feat: Relates to #337. Part 1 of 2 - Account view - Change header account design by ltfschoen · Pull Request #363 · openethereum/fether

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

Part 1 of 2 - Account View (see first wireframe #337)

This PR just covers the first wireframe of issue #337.
Note that a separate PR will be made for Part 2 of 2 - Transaction Recap

Change Account screen so its header uses the Information component with a big identicon, a short address,
so it is clickable, and with identicon on left of name and address.
Apply extra bottom margin between name and account address when Information component used in the header
Update header so it uses Flexbox according to docs as intended
Since we're using Flexbox without placeholder spacing for the left and right header buttons, when the title is shown in the header it is offset if there is only a left or right button. So a titleOffset prop
is used on pages where the Header component is used to indicate if the title needs to be offset to the left or right so it appears centered in the screen. We also pass a screen prop (named after the component where the Header component is being used so we can tailor the header spacing on specific screens. EDIT: I've pushed commits to use placeholder spacing for the header areas: i.e. left (button), middle (title), and right (button) since I think it is a much cleaner solution~~
Re-order props alphabetically
QUESTION - should we consider always adding placeholder spacing for where both right and left buttons in header would be as an alternative, so no offset for the title is require? Luke: Yes, much cleaner solution

…ount design * Change Account screen so its header uses the Information component with a big identicon, a short address, so it is clickable, and with identicon on left of name and address. * Apply extra bottom margin between name and account address when Information component used in the header * Update header so it uses Flexbox according to docs as intended * Since we're using Flexbox without placeholder spacing for the left and right header buttons, when the title is shown in the header it is offset if there is only a left or right button. So a `titleOffset` prop is used on pages where the Header component is used to indicate if the title needs to be offset to the left or right so it appears centered in the screen. We also pass a `screen` prop (named after the component where the Header component is being used so we can tailor the header spacing on specific screens. * Re-order props alphabetically * QUESTION - should we consider always adding placeholder spacing for where both right and left buttons in header would be as an alternative so no offset for the title is require?

Tbaut

Thanks for the PR.

The title need a little more offset as you can see here, the capital "B" is cut:

should we consider always adding placeholder spacing for where both right and left buttons in header would be as an alternative, so no offset for the title is require?

We could center the account -header-container, I tried with a margin:auto and I think it looks good. Also once #356 is merged we'll always have a button on the right (the 3 dots) so that the header has a good symmetry.

ltfschoen · 2019-01-18T07:53:41Z

Also once #356 is merged we'll always have a button on the right (the 3 dots) so that the header gicing a good symmetry..

#356 is just the menu for the page of an Account.
what menu items will be in the menu that is always shown?

Tbaut · 2019-01-18T10:51:35Z

Not sure I follow you. This PR is only for the account view. #356 as well. The latter will introduce a "3 dots" menu button that will be on the right of the header. Having the "back arrow" on the left of the header, the account icon, name and address in the center and the "3 dots" menu introduced by #356 on the right gives a good symetry to the header.

^ I expected this to answer your quesion

should we consider always adding placeholder spacing for where both right and left buttons in header would be as an alternative, so no offset for the title is require?

but i might miss your point.

* Remove titleOffset and add placeholder width for header nav buttons * Remove passing `screen` prop for specific pages

…r title left * Remove useless .header-container * Add width to .account_information to fit full title width * Use `flex-grow` instead of abbreviation `flex` * Use justify-content: left to align account info and avatar to left and remove flex: 0 from .header-tokens

ltfschoen · 2019-01-19T08:47:29Z

Not sure I follow you. This PR is only for the account view. #356 as well. The latter will introduce a "3 dots" menu button that will be on the right of the header. Having the "back arrow" on the left of the header, the account icon, name and address in the center and the "3 dots" menu introduced by #356 on the right gives a good symetry to the header.

^ I expected this to answer your quesion

should we consider always adding placeholder spacing for where both right and left buttons in header would be as an alternative, so no offset for the title is require?

but i might miss your point.

@Tbaut I just cleaned up the code that I thought I'd already cleaned up...
I've used flexbox to structure the header so there is always a left, middle (title), and right area. The left and right area both have the same width, so if we want a left (i.e. "Back") or right button (i.e. "Next") shown on a page they'll always be in a consistent position.
I think this is much better than having to add messy offsets if there is a title and only a left or right button depending on what page they are on.

You can preview how I've structured it by temporarily modifying the CSS in paritytech/fether/packages/fether-react/src/assets/sass/components/_header-nav.scss by adding a border around the different header elements and then changing to different screens to see that it stays consistent.

  .header-nav_left,
  .header-nav_right {
    border: 1px solid red;
...
  }
...
  .header-nav_title {
    border: 1px solid blue;
...

Below is a screenshot of some pages, if you change page the red/blue borders will be in the same position.

Tbaut · 2019-01-21T15:34:14Z

👍 Can you make sure the account, that's inside the blue part in you screenshot is centred?
This would make things more consistent (all titles in this area are centred).

ltfschoen · 2019-01-21T18:45:56Z

@Tbaut I've pushed changes to center the account identicon/name/address in the header and you can still click anywhere in title area to copy

Tbaut

👍 jawohl!

amaury1093

Nice! One small comment on renaming a prop

amaury1093 · 2019-01-22T10:43:43Z

packages/fether-react/src/Accounts/AccountsList/AccountsList.js

                      className='-clickable'
                      type={accountsInfo[address].type}
                      name={accountsInfo[address].name || '(no name)'}
+                      screen='accounts'


This prop only decides if the account info is justified on the left or in the center, right? In this case, can we rename this prop to justify='left' | 'center', or isCentered={true|false}? It seems weird that a prop says on which screen the component is located.

There is only hard-coded styles deciding if the account info is justified left or center.
There is an account -header style that's used here for the header on the "Account page" https://github.com/paritytech/fether/blob/5d620bed8f0d55d00fc678cfa4c0cabadf7550a0/packages/fether-ui/src/AccountHeader/AccountHeader.js#L22. We use the className account -header so it uses justify center as defined here https://github.com/paritytech/fether/blob/5d620bed8f0d55d00fc678cfa4c0cabadf7550a0/packages/fether-react/src/assets/sass/components/_account.scss#L5, otherwise if it were just account (as we use on the "Account List page" then it's justified to the left by default (but we don't have to write it explicitely.

The prop screen='accounts' that you mentioned is actually passed down to the Name component associated with the AccountCard since it's used both in the body and header components of the Fether window. The "Accounts List page" is the only page that has a top and bottom margin that makes the Information component appear good (which contains the Name and Address component). However other pages that it's used are in the header area, but have insufficient top and bottom margin, as shown below (see how there is not much gap between "scon" and "0x00C5...31F5", and they both appear too high:

So in the Name component I've use this logic className={account_name ${screen !== 'accounts' ? '-header' : ''}} https://github.com/paritytech/fether/pull/363/files#diff-1f4b2128964b51e8a48e4ee7c84a3e9fR13 so it applies the className account_name -header if the Name component is being used in the header area (which applies some margin above and below the name so the overall Information component in the header appears the same as where it's used on the "Accounts List page" https://github.com/paritytech/fether/pull/363/files#diff-1d583cfb15e661b39f28b9b82fa329c0R47) as shown below:

Otherwise it just uses the className accounts_name.

I understand, my proposal was to rename the screen props to justify, which I think describes better the intent:

className={`account_name ${justify === 'left' ? '-header' : ''}`}

if you think screen name fits better, then we can also keep it.

I understand, my proposal was to rename the screen props to justify, which I think describes better the intent:

className={`account_name ${justify === 'left' ? '-header' : ''}`}

if you think screen name fits better, then we can also keep it.

I understand what you're saying, but I don't think I made myself clear enough in my last message. The ternary logic that I've added to the Name component only determines whether to apply some top/bottom padding to the Name component, (not to apply any left/center justification), so I don't think justify is the right word, since I'm not using it to justify any left/right alignment.

I think meaningful alternatives to the word screen would be isPageWithoutCustomVerticalPadding or similar

amaury1093

lgtm!

ltfschoen added A0-pleasereview Z1-question labels Jan 16, 2019

Merge branch 'master' into luke-337-screen-address-lengths

2b394bc

Tbaut reviewed Jan 16, 2019

View reviewed changes

Tbaut added A5-grumble and removed A0-pleasereview Z1-question labels Jan 16, 2019

review-fix: Add top margin to address so it doesn't crop at top

005df87

review-fix: Remove titleOffset nad instead

37c6057

* Remove titleOffset and add placeholder width for header nav buttons * Remove passing `screen` prop for specific pages

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

ltfschoen added 3 commits January 18, 2019 23:14

Merge branch 'master' into luke-337-screen-address-lengths

67e14f8

Merge branch 'master' into luke-337-screen-address-lengths

eb39780

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

ltfschoen added 2 commits January 22, 2019 05:34

fix: Align account identicon and address in center

eaa1128

Merge branch 'master' into luke-337-screen-address-lengths

5d620be

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

Tbaut approved these changes Jan 22, 2019

View reviewed changes

Tbaut requested a review from amaury1093 January 22, 2019 09:25

amaury1093 reviewed Jan 22, 2019

View reviewed changes

amaury1093 added the A5-grumble label Jan 22, 2019

amaury1093 removed the A0-pleasereview label Jan 22, 2019

ltfschoen added the A0-pleasereview label Jan 22, 2019

amaury1093 approved these changes Jan 22, 2019

View reviewed changes

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

amaury1093 merged commit 7616e99 into master Jan 22, 2019

amaury1093 deleted the luke-337-screen-address-lengths branch January 22, 2019 21:53

Conversation

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

Uh oh!

Part 1 of 2 - Account View (see first wireframe #337)

Uh oh!

Tbaut 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!

ltfschoen commented Jan 18, 2019

Uh oh!

Tbaut commented Jan 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

Tbaut commented Jan 21, 2019

Uh oh!

ltfschoen commented Jan 21, 2019

Uh oh!

Tbaut left a comment

Choose a reason for hiding this comment

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

amaury1093 Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

ltfschoen Jan 22, 2019 • 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 22, 2019

Choose a reason for hiding this comment

Uh oh!

ltfschoen Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ltfschoen commented Jan 16, 2019 •

edited

Loading

Tbaut left a comment •

edited

Loading

ltfschoen commented Jan 19, 2019 •

edited

Loading

ltfschoen Jan 22, 2019 •

edited

Loading