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

Fix extension detection #6452

Merged
merged 6 commits into from
Sep 15, 2017
Merged

Fix extension detection #6452

merged 6 commits into from
Sep 15, 2017

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Sep 4, 2017

Sometimes the extension script is injected a little bit late, so we need to defer the check.
Resolves #6388

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Sep 4, 2017
@tomusdrw tomusdrw requested a review from jacogr September 4, 2017 07:22
@@ -22,6 +22,7 @@
height: 100%;
position: relative;
width: 100%;
max-width: 100%;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes display of accounts on firefox (especially in Parity Bar when selecting default account, the accounts were overlapping).

@gavofyork gavofyork added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 5, 2017
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 5, 2017
@@ -60,19 +60,27 @@ export default class Store {
}

@action testInstall = () => {
this.shouldInstall = this.readStatus();
this.readStatus()
Copy link
Contributor

Choose a reason for hiding this comment

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

MobX sometimes does things a bit funny when dealing with promises, i.e. not firing updates when inside a promise chain. Generally it would be preferable to...

@action setInstallStatus = (shouldInstall) => {
  this.shouldInstall = shouldInstall;
}

testInstall = () => {
  this.readStatus().then(this.setInstallStatus);
}

Copy link
Contributor

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Generally looks good, however I would swap the state updates to be inside a function and not as part of the promise chain. Updates may/may not fire in the current incarnation.

@tomusdrw
Copy link
Collaborator Author

Thanks @jacogr! Updated.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Sep 13, 2017
@jacogr
Copy link
Contributor

jacogr commented Sep 13, 2017

LG happy to merge, however Rust tests failing (not that there were any changes there). Something weird is going on.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 15, 2017
@gavofyork gavofyork merged commit b602fb4 into master Sep 15, 2017
@gavofyork gavofyork deleted the td-extension-fixes branch September 15, 2017 13:06
tomusdrw added a commit that referenced this pull request Sep 15, 2017
* Fix extension detection.

* Fix mobx quirks.

* Update submodule.
gavofyork pushed a commit that referenced this pull request Sep 15, 2017
* Fix extension detection.

* Fix mobx quirks.

* Update submodule.
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.

3 participants