Embeddable ParityBar #4222

tomusdrw · 2017-01-19T16:07:17Z

Some changes required to deploy injectable ParityBar in Chrome Extension.

tomusdrw · 2017-01-19T16:09:31Z

Probably it would be good to include embed.js in parity.js on NPM to use it in extension (I will look into it)

derhuerst · 2017-01-19T16:17:32Z

js/src/redux/middleware.js

+export default function (api, browserHistory, forEmbed = false) {
+  let middleware;
+  if (forEmbed) {
+    const errors = new ErrorsMiddleware();


Move the common middle out of the if/else blocks. Use concat to add middleware.

jacogr · 2017-01-19T16:29:38Z

js/src/secureApi.js

@@ -56,8 +62,8 @@ export default class SecureApi extends Api {
  }

  get hostname () {
-    if (window.location.hostname === 'home.parity') {
-      return 'dapps.parity';
+    if (window.location.hostname === 'home.ethlink.io') {


Not 100% sure what this is doing here, however...

This is dependent on the ethlink branch (should be merged first)

We have moved to web3.site

We still have the proxy under settings, so this breaks that

Sorry, included by mistake.

jacogr · 2017-01-19T16:31:00Z

js/src/util/storage.js

+/**
+ * A safe localStorage wrapper
+ */
+class Storage {


Instead of doing this, perfer the use of store, i.e. import store from 'store';

Consistent with what we use elsewhere instead of adding yet another module

No need for try/catch

It does nice things like auto to/from JSON

I just replaced all occurences of window.localStorage with this, cause localStorage is not available in chrome extension. Hopefuly store will solve that too, will fix :)

jacogr · 2017-01-19T16:31:30Z

js/src/ui/Tooltips/reducers.js

@@ -14,13 +14,15 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.

+import storage from '~/util/storage';


import store from 'store';

jacogr · 2017-01-19T16:31:37Z

js/src/views/Application/store.js

@@ -15,14 +15,15 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.

 import { action, observable } from 'mobx';
+import storage from '~/util/storage';


import store from 'store';

coveralls · 2017-01-19T19:39:08Z

Coverage increased (+0.008%) to 84.871% when pulling 9f01f2f on embed into 5830e03 on master.

jacogr · 2017-01-20T18:39:23Z

js/src/views/ParityBar/parityBar.js

@@ -96,6 +117,23 @@ class ParityBar extends Component {
    );
  }

+  renderLink (button) {
+    const { externalLink } = this.props;
+    if (!externalLink) {


Prefer extra line break here https://github.com/ethcore/parity/pull/4234

jacogr · 2017-01-20T18:39:51Z

js/src/views/ParityBar/parityBar.js

+    }
+
+    return (
+      <a href={ externalLink } target='_parent'>


When doing multiple attributes, spreading them one per line is preferable.

jacogr · 2017-01-20T18:41:54Z

js/src/embed.js

+
+injectTapEventPlugin();
+
+import ParityBar from './views/ParityBar';


Mixture of ~ (i.e. ui as used above) and ./ - doesn't make much difference here since it sits at the root, just noting it that generally we prefer ~ and if things are moved, it won't have any impact.

jacogr · 2017-01-20T18:42:27Z

js/src/embed.js

+    console.log('Calling', method, params);
+    return Promise.reject('not connected');
+  }
+  on () {}


Blank line before method & prefer

on () { }

jacogr · 2017-01-20T18:43:04Z

js/src/embed.js

+}
+
+const api = new FrameSecureApi(window.secureTransport || new FakeTransport());
+patchApi(api);


newline after var declaration. ()Not trapped yet)

jacogr · 2017-01-20T18:43:11Z

js/src/embed.js

+ContractInstances.create(api);
+
+const store = initStore(api, null, true);
+store.dispatch({ type: 'initAll', api });


jacogr · 2017-01-20T18:43:23Z

js/src/embed.js

+);
+
+const container = document.createElement('div');
+document.body.appendChild(container);


jacogr · 2017-01-20T18:43:46Z

js/src/redux/middleware.js

+  }
+
+  const status = statusMiddleware();
+  const routeMiddleware = browserHistory ? routerMiddleware(browserHistory) : [];


newline after.

jacogr · 2017-01-20T18:44:18Z

js/src/secureApi.js

@@ -18,9 +18,9 @@ import { uniq } from 'lodash';

 import Api from './api';
 import { LOG_KEYS, getLogger } from '~/config';
+import store from 'store';


Generally prefer package imports to go at the top, then local src, then local to component.

jacogr · 2017-01-20T18:44:28Z

js/src/secureApi.js

+
+  constructor (url, nextToken, getTransport = SecureApi.getTransport) {
+    const sysuiToken = store.get('sysuiToken');
+    const transport = getTransport(url, sysuiToken);


newline after.

jacogr · 2017-01-20T18:45:40Z

js/src/views/ParityBar/parityBar.js

@@ -29,7 +29,8 @@ import styles from './parityBar.css';
 class ParityBar extends Component {
  static propTypes = {
    pending: PropTypes.array,
-    dapp: PropTypes.bool
+    dapp: PropTypes.bool,


Bad example since this was not done, but we do prefer to re-arrange alphabetically when touching (have not done a bulk-re-arrange, but when touching we do do it.)

jacogr · 2017-01-20T18:46:15Z

js/src/views/ParityBar/parityBar.js

+      bubbles: true,
+      detail: { opened }
+    });
+    this.bar.dispatchEvent(event);


newline before. (As per slightly stricter rules - things are growing, just trying to keep consistency going)

jacogr · 2017-01-20T18:46:59Z

js/src/views/ParityBar/parityBar.js

+    this.bar.dispatchEvent(event);
+  }
+
+  onRef = (el) => {


Would have preferred slightly more descriptive variable naming, i.e. element (we gain understanding reading, lose nothing after compilation)

jacogr · 2017-01-20T18:47:10Z

js/src/views/ParityBar/parityBar.js

@@ -72,18 +92,19 @@ class ParityBar extends Component {
        className={ styles.parityIcon }
      />
    );
+    const parityButton = (


newline before.

jacogr · 2017-01-20T18:47:24Z

js/src/views/ParityBar/parityBar.js


    return (
-      <div className={ styles.bar }>
+      <div className={ styles.bar } ref={ this.onRef }>


Split multiple attributes to one per line.

jacogr · 2017-01-20T18:47:48Z

js/src/views/Settings/middleware.js

@@ -15,6 +15,7 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.

 import defaultViews from './Views/defaults';
+import store from 'store';


package imports should go at the top.

jacogr · 2017-01-20T18:48:37Z

Code style comments, but overall no serious issues or bugs found.

ngotchac · 2017-01-20T18:52:16Z

js/src/embed.js

+
+// Cleanup Loading
+const $container = document.querySelector('#container');
+$container.parentNode.removeChild($container);


Why not having React attached to the #container directly ?

ngotchac · 2017-01-20T18:58:45Z

Looks good to me !

gavofyork · 2017-01-21T13:46:30Z

style grumbles and conflict but good to merge otherwise.

Conflicts: js/src/views/ParityBar/parityBar.js

tomusdrw · 2017-01-22T15:55:47Z

Style issues addressed, thanks for pointing them out 👍

* Embeddable ParityBar * Replacing storage with store * Fixing references. * Addressing style issues * Supporting parity background Conflicts: js/src/views/ParityBar/parityBar.js

Embeddable ParityBar

9f01f2f

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 19, 2017

derhuerst reviewed Jan 19, 2017

View reviewed changes

jacogr reviewed Jan 19, 2017

View reviewed changes

gavofyork 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 20, 2017

Tomasz Drwięga added 2 commits January 20, 2017 14:10

Replacing storage with store

096361e

Fixing references.

5872036

tomusdrw 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 20, 2017

jacogr reviewed Jan 20, 2017

View reviewed changes

js/src/embed.js

);

const container = document.createElement('div');

document.body.appendChild(container);

Copy link

Contributor

jacogr Jan 20, 2017

Choose a reason for hiding this comment

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

newline.

jacogr reviewed Jan 20, 2017

View reviewed changes

ngotchac reviewed Jan 20, 2017

View reviewed changes

gavofyork 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 21, 2017

Tomasz Drwięga added 2 commits January 22, 2017 16:30

Merge branch 'master' into embed

69afa9c

Conflicts: js/src/views/ParityBar/parityBar.js

Addressing style issues

0159eaa

tomusdrw 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 22, 2017

Supporting parity background

b22967e

tomusdrw mentioned this pull request Jan 23, 2017

Rough version of web3 support and embeddable ParityBar paritytech/parity-extension#16

Merged

4 tasks

ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 23, 2017

jacogr merged commit 3e70e88 into master Jan 23, 2017

jacogr deleted the embed branch January 23, 2017 12:04

tomusdrw added a commit that referenced this pull request Jan 24, 2017

Embeddable ParityBar (#4222)

27d95ec

* Embeddable ParityBar * Replacing storage with store * Fixing references. * Addressing style issues * Supporting parity background Conflicts: js/src/views/ParityBar/parityBar.js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embeddable ParityBar #4222

Embeddable ParityBar #4222

tomusdrw commented Jan 19, 2017

tomusdrw commented Jan 19, 2017

derhuerst Jan 19, 2017

jacogr Jan 19, 2017

tomusdrw Jan 20, 2017

jacogr Jan 19, 2017

tomusdrw Jan 20, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

coveralls commented Jan 19, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr Jan 20, 2017

jacogr commented Jan 20, 2017

ngotchac Jan 20, 2017

ngotchac commented Jan 20, 2017

gavofyork commented Jan 21, 2017

tomusdrw commented Jan 22, 2017


		injectTapEventPlugin();

		import ParityBar from './views/ParityBar';

Embeddable ParityBar #4222

Embeddable ParityBar #4222

Conversation

tomusdrw commented Jan 19, 2017

tomusdrw commented Jan 19, 2017

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

coveralls commented Jan 19, 2017

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 Jan 20, 2017

Choose a reason for hiding this comment

ngotchac commented Jan 20, 2017

gavofyork commented Jan 21, 2017

tomusdrw commented Jan 22, 2017