Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

3box integration 2.0 #6972

Merged
merged 61 commits into from
Sep 16, 2019
Merged

3box integration 2.0 #6972

merged 61 commits into from
Sep 16, 2019

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Aug 7, 2019 •
edited
Loading

This PR adds on to the current 3box-integration PR. Changes include:

  • switch to use spaces api instead of profile api
  • ensures that state is only restored on import, and only after the user confirms via a modal prompt

e2e tests are in development, but will be added in a future PR

Demo here: https://streamable.com/ovvbf

@danjm danjm requested review from Gudahtt and whymarrh as code owners August 7, 2019 13:04
@danjm danjm force-pushed the 3box-integration branch from 3bf6c47 to 8d8b58b Compare August 7, 2019 16:21
@danjm danjm force-pushed the 3box-integration-2.0 branch from 1023398 to e2d086f Compare August 7, 2019 16:54
@metamaskbot
Copy link
Collaborator

Builds ready [c92ed60]: chrome, firefox, edge, opera

@bdresser
Copy link
Contributor

bdresser commented Aug 7, 2019

This looks great! Small things:

  • I think we need to update the button styles to mtach the design system:

Screen Shot 2019-08-07 at 3 11 51 PM

Screen Shot 2019-08-07 at 3 12 27 PM

  • Could primary button say "Restore"

@danjm
Copy link
Contributor Author

danjm commented Aug 13, 2019

@bdresser

I made the threebox modal consistent with other modals while updating the wording to restore:

Screenshot from 2019-08-13 01-46-01
Screenshot from 2019-08-13 01-20-44
Screenshot from 2019-08-13 01-20-25

All of these modals have different buttons from those that you shared a screen shot of above. If the design system specifies a change to these modals design, we can do them all at once in a separate issue.

@danjm danjm force-pushed the 3box-integration-2.0 branch 3 times, most recently from 15514dd to 0d1be18 Compare August 13, 2019 16:02
@bdresser
Copy link
Contributor

cool, filed here, can handle later.

where did the ? come from in the first line? let's cut it please

Screen Shot 2019-08-13 at 9 04 32 AM

@danjm danjm force-pushed the 3box-integration branch from 8d8b58b to 9ec437d Compare August 13, 2019 16:14
@danjm danjm force-pushed the 3box-integration-2.0 branch from 0d1be18 to 7f13792 Compare August 13, 2019 16:15
@metamaskbot
Copy link
Collaborator

Builds ready [7f13792]: chrome, firefox, edge, opera

@danjm
Copy link
Contributor Author

danjm commented Aug 13, 2019

where did the ? come from in the first line? let's cut it please

Cut with 8363564

I my first job a senior engineer told me that copy+paste is the devil. Its times like these I believe it...

@metamaskbot
Copy link
Collaborator

Builds ready [8363564]: chrome, firefox, edge, opera

@danjm danjm changed the base branch from 3box-integration to develop August 15, 2019 13:36
@danjm danjm force-pushed the 3box-integration-2.0 branch from 8363564 to 4cc7c4a Compare August 15, 2019 15:49
@metamaskbot
Copy link
Collaborator

Builds ready [4cc7c4a]: chrome, firefox, edge, opera

Gudahtt
Gudahtt reviewed Aug 16, 2019
View reviewed changes
ui/app/ducks/app/app.js Outdated Show resolved Hide resolved
Gudahtt
Gudahtt reviewed Aug 16, 2019
View reviewed changes
ui/app/store/actions.js
@@ -2798,3 +2825,79 @@ function setSeedPhraseBackedUp (seedPhraseBackupState) {
})
}
}

function hideSeedPhraseBackupAfterOnboarding () {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this was added accidentally.

Copy link
Member

@Gudahtt Gudahtt Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait... nevermind. This is needed - it was just missing. That's concerning 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, it is not needed. The case where it appears in the code is never called. I've removed the addition of this via a rebase, and will remove the case where it is called in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is still here - did it get re-added?

Copy link
Member

@Gudahtt Gudahtt Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this should be here - it's just broken on develop right now 😕

Gudahtt
Gudahtt reviewed Aug 16, 2019
View reviewed changes
ui/app/ducks/app/app.js Outdated Show resolved Hide resolved
@danjm danjm force-pushed the 3box-integration-2.0 branch 2 times, most recently from 5e641f1 to 035eb23 Compare September 11, 2019 19:16
@metamaskbot
Copy link
Collaborator

Builds ready [035eb23]

Gudahtt
Gudahtt requested changes Sep 11, 2019
View reviewed changes
app/scripts/migrations/037.js Outdated Show resolved Hide resolved
gulpfile.js Outdated
...json.background,
scripts: json.background.scripts.filter(scriptName => !scriptsToExcludeFromBackgroundDevBuild[scriptName]),
}
json.permissions = [...json.permissions, 'webRequestBlocking', 'http://localhost/8889']
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on whether we should change this to localhost/*? I remain confused about why localhost/8999 works in the first place. If you feel inclined to leave it as localhost/8999 we should at least make sure it works on Firefox, as I can't see any documentation to explain why it does work, so it could be some sort of fluke.

ui/app/store/actions.js Outdated Show resolved Hide resolved
ui/app/pages/home/home.component.js Outdated
@@ -125,6 +152,25 @@ export default class Home extends PureComponent {
key="home-backupApprovalNotice"
/>,
},
{
shouldBeRendered: threeBoxLastUpdated && restoredFromThreeBox === null,
Copy link
Member

Choose a reason for hiding this comment

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

Conditions like this might leave us in a strange state if the flag is turned on, then off again.

This situation wouldn't occur unless someone enabled it in the dev console of course, but do you think it's worth adding the flag here to prevent it anyway? It's possible an end-user might enable 3Box in the dev console then disable it, and get in a weird state like this. Maybe unlikely though - I'm OK with ignoring this case if you'd prefer. If you did want to handle it, there are a few other examples around like this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done in 4fcc18a

app/scripts/metamask-controller.js Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Sep 11, 2019

Also it'd be nice to have tests for the migration. I think all of the recent migrations have been tested, so it should be pretty easy to add using a previous one as a template.

@danjm danjm force-pushed the 3box-integration-2.0 branch from 035eb23 to 3d81850 Compare September 12, 2019 01:50
@danjm
Copy link
Contributor Author

danjm commented Sep 12, 2019

@Gudahtt I realized we don't actually need a migration. The feature flag will be undefined until it is set to true, which will have the same result as if we migrated to explicitly be false.

@metamaskbot
Copy link
Collaborator

Builds ready [3d81850]

Gudahtt
Gudahtt approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good!

@danjm danjm merged commit 7985f4f into develop Sep 16, 2019
Gudahtt added a commit that referenced this pull request Sep 17, 2019
@Gudahtt
Merge remote-tracking branch 'origin/develop' into sync-master-with-d…
dcc38dd 
…evelop

* origin/develop: (31 commits)
  Performance: Delivery optimized images (#7176)
  Add `appName` message to each locale
  Remove the disk store (#7170)
  Update @hapi/subtext as per security advisory (#7172)
  Add fixes for German translations (#7168)
  Fix recipient field of approve screen (#7171)
  3box integration 2.0 (#6972)
  ci - metamaskbot - include links to dep-viz and all artifacts (#7155)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Remove redundant error logging (#7158)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  ci - install deps with "--har" flag to capture network activity (#7143)
  ci - create source-map-explorer build-artifacts (#7141)
  ci - build-artifacts - generate sesify-viz for inspecting deps (#7151)
  Publish GitHub release from master branch (#7136)
  fix rinkeby spelling (#7148)
  deps - move gulp-terser-js to devDeps
  test:integration - fix renamed test data file
  lint fix
  ...
@danjm danjm deleted the 3box-integration-2.0 branch October 2, 2019 18:14
@Gudahtt Gudahtt mentioned this pull request Oct 17, 2019
Closed
@bbondy bbondy mentioned this pull request Dec 16, 2019
Closed
@Gudahtt Gudahtt mentioned this pull request Oct 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danfinlay danfinlay danfinlay left review comments

@Gudahtt Gudahtt Gudahtt approved these changes

@whymarrh whymarrh Awaiting requested review from whymarrh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danjm @metamaskbot @bdresser @Gudahtt @danfinlay @whymarrh