Several Fixes to the UI #3799

ngotchac · 2016-12-10T18:25:30Z

This PR includes these fixes:

Fix dapps separation: the margin between different kind of dapps was wrong/inexistant
Use React Router Link component for App Tabs: this enables the user to Ctrl+Click on a tab to open the page in a new tab (which wasn't the case previously). The tab active state is also handled by React Router, so no more map of URL path to Tab ; all is in the Router definition
New routes : use /addresses/:address instead of /address/:address. This enables React Router to properly manage the tabs active state. The old routes are still working, only a warning log is printed in the console
Fix flash of unsorted/unnamed accounts in address book while the app is loading
Prevent creating new Contracts instances, which was created by SMS Verification store (thus, contract lookups were occurring at each loading of an account page)
Enable loading of an external account page : going to /addresses/:unknownAddress shows informations about the :unknownAddress instead of a blank page. The user can then add it to his address book
Add balances to the accounts in account selection component in modals where an account needs to be selected to send a transaction (wallet transfer, contracts). This should prevent selecting an account with insufficient funds
Use the new onClose prop of the Autocomplete instead of a onBlur hack => better closing of autocomplete
Add dividers to Autocomplete : see the transfer modal, selecting a to address

jacogr · 2016-12-10T18:35:44Z

js/src/contracts/contracts.js

@@ -62,6 +62,10 @@ export default class Contracts {
  }

  static create (api) {
+    if (instance) {


We still don't seem to assign the new instance to the instance variable.

This is done in the Contracts constructor

jacogr · 2016-12-10T18:37:00Z

js/src/main.js

+
+          <Route path='contracts'>
+            <IndexRoute component={ Contracts } />
+            <Route path='write' component={ WriteContract } />


While we are renaming and fixing up - the "write contract" is very confusing. Had 2 people asking me now where the find the editor.

"Develop Contract" ?

Done (renamed the button)

jacogr · 2016-12-10T18:40:09Z

js/src/ui/Form/AddressSelect/addressSelect.js

+      return null;
+    }
+
+    const ethToken = balance.tokens.find((t) => t.token.tag.toLowerCase() === 'eth');


Naming is different from the tok.token in balancesReducer.

No nulls here?

jacogr · 2016-12-10T18:40:37Z

js/src/ui/Form/AddressSelect/addressSelect.js

@@ -139,6 +187,8 @@ export default class AddressSelect extends Component {
        <IdentityName
          className={ styles.name }
          address={ address } />
+


Any reason for the extra space?

jacogr · 2016-12-10T18:41:45Z

js/src/ui/Form/AutoComplete/autocomplete.js

 import { PopoverAnimationVertical } from 'material-ui/Popover';

 import { isEqual } from 'lodash';

+// Hack to prevent "Unknown prop `disableFocusRipple` on <hr> tag" error
+class Divider extends Component {


Split components into their own files. Started doing that as cleanups where I've found them, the 100's of lines in a file just makes it very difficult to maintain.

Agreed. However this is just a Hack because there is a bug in Material UI (using directly <Divider /> spits out Unknown prop disableFocusRipple on <hr> tag error logs). This will be deleted when a fix is out

jacogr · 2016-12-10T18:43:56Z

js/src/modals/AddAddress/addAddress.js

@@ -28,6 +28,7 @@ export default class AddAddress extends Component {

  static propTypes = {
    contacts: PropTypes.object.isRequired,
+    address: PropTypes.string,


General comment. When changing these (or adding, removing), it would make sense to start putting them in alphabetical order since you are in there already. (It goes for everywhere - low overhead). This one should have been on-top as an example.

So here we separate the isRequired props to those optional. I think it's better to be able to see which props should absolutely be passed to a Component easily. Within those two categories, I agree they could be sorted from a-z

Well, we don't conform to that at all currently. With the Redux issues and not being able to pass required props this whole methodology falls apart. (e.g. see isTest which is absolutely required and just currently needs to be moved to non-required since it just errors all-over)

So there are 3 categories, Required, non-required & Redux. It just becomes an absolute mess and mental overhead to manage.

I agree that it should be split into 3 categories. In the meanwhile, I prefer splitting in at least 2.

jacogr · 2016-12-10T18:45:10Z

js/src/modals/DeployContract/DetailsStep/detailsStep.js

@@ -37,6 +37,7 @@ export default class DetailsStep extends Component {
    onParamsChange: PropTypes.func.isRequired,
    onInputsChange: PropTypes.func.isRequired,

+    balances: PropTypes.object,


Same comment here as above (last time, I promise) - rather than trying arbitrary spacings between props, just sort and condense. Easy to find without understanding what the developer had in mind as a category system.

Same comment as above ;)

jacogr · 2016-12-10T18:45:47Z

js/src/modals/DeployContract/DetailsStep/detailsStep.js

@@ -97,6 +99,7 @@ export default class DetailsStep extends Component {
          value={ fromAddress }
          error={ fromAddressError }
          accounts={ accounts }
+          balances={ balances }


Same sorting goes for passed props where touched.

jacogr · 2016-12-10T18:46:23Z

js/src/modals/ExecuteContract/DetailsStep/detailsStep.js

+    onValueChange: PropTypes.func.isRequired,
+    values: PropTypes.array.isRequired,
+    valuesError: PropTypes.array.isRequired,
+


extra blank line for sorting, remove.

Same as above

jacogr · 2016-12-10T18:48:43Z

js/src/views/Account/account.js

@@ -26,7 +26,7 @@ import VerifyIcon from 'material-ui/svg-icons/action/verified-user';
 import { EditMeta, DeleteAccount, Shapeshift, SMSVerification, Transfer, PasswordManager } from '~/modals';
 import { Actionbar, Button, Page } from '~/ui';

-import shapeshiftBtn from '../../../assets/images/shapeshift-btn.png';
+import shapeshiftBtn from '~/../assets/images/shapeshift-btn.png';


Great that it works.

jacogr · 2016-12-10T18:50:47Z

js/src/views/Addresses/addresses.js

+
+    if (hasContacts && Object.keys(balances).length === 0) {
+      return (
+        <Loading size={ 3 } />


Worried about the loading sizes everywhere - should pick one size and just stick with it throughout. (Only exceptions may be whole pages where everything is blank)

Well, I think size of loader should depend on where it is used. If it's displayed while loading the full app, it should be bigger than when loading MethodDecoding for example.

Agreed. However in the UI itself it should be 100% consistent. So for the dapp example (not that it is used, agreed) - for the rest, all the same. This doesn't conform to the 2 used elsewhere since the introduction.

jacogr · 2016-12-10T19:00:51Z

Merge in master for gitlab build checks.

coveralls · 2016-12-10T19:02:00Z

Coverage decreased (-0.003%) to 85.6% when pulling 0f6681d on ng-ui-fixes into a6fcd8a on master.

coveralls · 2016-12-10T20:48:04Z

Coverage increased (+0.01%) to 85.613% when pulling 70eab0d on ng-ui-fixes into 08a47ea on master.

coveralls · 2016-12-10T20:55:17Z

Coverage remained the same at 85.601% when pulling 70eab0d on ng-ui-fixes into 08a47ea on master.

coveralls · 2016-12-10T22:34:44Z

Coverage increased (+0.006%) to 85.607% when pulling e5c73b2 on ng-ui-fixes into 08a47ea on master.

coveralls · 2016-12-10T23:10:27Z

Coverage remained the same at 85.601% when pulling e5c73b2 on ng-ui-fixes into 08a47ea on master.

coveralls · 2016-12-10T23:46:51Z

Coverage increased (+0.009%) to 85.609% when pulling e5c73b2 on ng-ui-fixes into 08a47ea on master.

ngotchac added 15 commits December 10, 2016 01:32

Fix dapps separation

f6564dc

Treat tabs as real link (enable Ctrl+Click for new Tab)

0d9b188

Don't show addresses while loading balances (fix flash of unsorted)

51b9034

Better use of React-Router (maintaining old routes)

591d086

Better use of Tab Bar

13607d4

Don't create new Contracts instance if already exists

81c5085

Fix tab bar active style

65f586e

See addresses outside address book + Save them

ef93262

Fix wallet data's prop types

054b481

Add sender balance in transfer modal

d9da8a4

Add sender balances to contract (exec/deploy)

8411613

Use the new onClose autocomplete prop

cd6ab07

Add dividers to AutoComplete

2346f29

Better Autocomplete Divider

69c0086

Linting issue

0f6681d

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 10, 2016

jacogr reviewed Dec 10, 2016

View reviewed changes

ngotchac added 2 commits December 10, 2016 20:23

Merge branch 'master' into ng-ui-fixes

89ed764

PR grumbles

70eab0d

ngotchac added 3 commits December 10, 2016 22:15

PR grumbles

7401358

Fix padding bottom // Status bar fixed

9dbccfd

'contract/write' to 'contract/develop'

e5c73b2

jacogr added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Dec 10, 2016

Merge branch 'master' into ng-ui-fixes

ba3bfcc

ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Dec 11, 2016

gavofyork merged commit 078feaa into master Dec 11, 2016

gavofyork deleted the ng-ui-fixes branch December 11, 2016 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several Fixes to the UI #3799

Several Fixes to the UI #3799

ngotchac commented Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

jacogr Dec 10, 2016

ngotchac Dec 10, 2016

jacogr Dec 10, 2016

jacogr commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

Several Fixes to the UI #3799

Several Fixes to the UI #3799

Conversation

ngotchac commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016