Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Update AccountCard for re-use #4350

Merged
merged 10 commits into from
Jan 31, 2017
Merged

Update AccountCard for re-use #4350

merged 10 commits into from
Jan 31, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 30, 2017

AccountCard updates -

  1. Component is made available in ~/ui
  2. Use standard ~/ui/Balance component for balance display (UI consistency, DRY code)
  3. Add className prop
  4. Make onClick & onFocus optional

Additional -

  1. ui/Balance support showOnlyEth & showZeroValues attributes
  2. ui/Tags now returns null when either invalid or zero length

Split from work on Dapp Account selection where the Portal is to be used.

parity 2017-01-30 11-12-25

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 30, 2017
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Only comment is that the account card should look closer the to the account header I think. Meaning: the identity icon should be vertically aligned on top, and the balances should be under the identity icon, horizontally aligned to the left of the component. At least that's what the other account cards and the account header look like

import styles from './balance.css';

class Balance extends Component {
static contextTypes = {
api: PropTypes.object
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering : is there any ESLint rule we could use here for this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should complain about missing semi colons. Not sure why it doesn't in those cases. (And since it hasn't been complaining, I've been very lax - not caring too much one way or another)

There may be a class-based rule, will keep it in mind for the next ESLint update.

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 30, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Jan 30, 2017

Sure, I honestly didn't want to rebuild the full card, just made it actually usable elsewhere so at least we have one less place where we render the same info while also not repeating ourselves here.

I would suggest logging those, when either of use have a gap could make sense getting it 100% aligned.

(And probably even replacing the Header/Summary? Only difference could be the amount of info displayed, and background being a Container?)

@ngotchac
Copy link
Contributor

ngotchac commented Jan 30, 2017

Well it's only a small change, and I don't see the rush of pulling this in with a UI we don't feel right about.

How about these small changes ?

diff --git a/js/src/ui/AccountCard/accountCard.css b/js/src/ui/AccountCard/accountCard.css
index 64b4807..fcf1522 100644
--- a/js/src/ui/AccountCard/accountCard.css
+++ b/js/src/ui/AccountCard/accountCard.css
@@ -20,8 +20,8 @@
   margin: 0.5em 0;
 
   display: flex;
-  flex-direction: row;
-  align-items: center;
+  flex-direction: column;
+  align-items: flex-start;
 
   background-color: rgba(0, 0, 0, 0.8);
 
@@ -53,6 +53,13 @@
   }
 }
 
+.infoContainer {
+  display: flex;
+  flex-direction: row;
+  margin-bottom: 0.5em;
+  width: 100%;
+}
+
 .description {
   font-size: 0.75em;
   color: rgba(255, 255, 255, 0.5);
diff --git a/js/src/ui/AccountCard/accountCard.js b/js/src/ui/AccountCard/accountCard.js
index 5616b03..61d88a3 100644
--- a/js/src/ui/AccountCard/accountCard.js
+++ b/js/src/ui/AccountCard/accountCard.js
@@ -58,25 +58,28 @@ export default class AccountCard extends Component {
         onFocus={ this.onFocus }
         onKeyDown={ this.handleKeyDown }
       >
-        <IdentityIcon address={ address } />
-        <div className={ styles.accountInfo }>
-          <div className={ styles.accountName }>
-            <IdentityName
-              address={ address }
-              name={ name }
-              unknown
-            />
+        <div className={ styles.infoContainer }>
+          <IdentityIcon address={ address } />
+          <div className={ styles.accountInfo }>
+            <div className={ styles.accountName }>
+              <IdentityName
+                address={ address }
+                name={ name }
+                unknown
+              />
+            </div>
+            { this.renderDescription(description) }
+            { this.renderAddress(address) }
           </div>
-          <Tags tags={ tags } />
-          { this.renderDescription(description) }
-          { this.renderAddress(address) }
-          <Balance
-            balance={ balance }
-            className={ styles.balance }
-            showOnlyEth
-            showZeroValues
-          />
         </div>
+
+        <Tags tags={ tags } />
+        <Balance
+          balance={ balance }
+          className={ styles.balance }
+          showOnlyEth
+          showZeroValues
+        />
       </div>
     );
   }

Renders:

image

@jacogr
Copy link
Contributor Author

jacogr commented Jan 31, 2017

Sure can add the above, feel free to apply, if not I'll see when I have a gap sometime.

@ngotchac
Copy link
Contributor

@jacogr Ok, pushed. Label as A0-please review so you can confirm the layout. Otherwise, looks good to me.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 31, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 31, 2017
@jacogr jacogr merged commit 223c474 into master Jan 31, 2017
@jacogr jacogr deleted the jg-accountcard-updates branch January 31, 2017 11:21
jacogr added a commit that referenced this pull request Feb 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants