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.

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

Only opening new tab on dapp request for unitialized wallets #25

Merged
merged 1 commit into from Jul 11, 2019

Conversation

@ryanml
Copy link
Member

ryanml commented Jul 11, 2019

This adds a proxy to the passed openPopup method for ProviderApprovalController

Provider approval requests will just open the welcome page in a new tab for users who have not onboarded/started MM, and will not attempt to create a notification

Provider approval functions as expected once user has created an account

@ryanml ryanml requested a review from bbondy Jul 11, 2019
@ryanml ryanml self-assigned this Jul 11, 2019
@bbondy
bbondy approved these changes Jul 11, 2019
if (hasOnboarded) {
this.optsOpenPopup()
} else {
chrome.tabs.create({ url: 'home.html' })

This comment has been minimized.

Copy link
@bbondy

bbondy Jul 11, 2019

Member

I think this URL should be chrome://wallet/home.html

This comment has been minimized.

Copy link
@ryanml

ryanml Jul 11, 2019

Author Member

@bbondy won't the URL rewrite core-side catch and rewrite it?

This comment has been minimized.

Copy link
@ryanml

ryanml Jul 11, 2019

Author Member

MM creates windows/tabs using relative paths

This comment has been minimized.

Copy link
@bbondy

bbondy Jul 11, 2019

Member

You can try, I'm ok if it works. I had seen that if you try to load the chrome-extension URL directly it doesn't show the Brave icon and verified brave page, but if you do use the chrome:// URL which shows as brave:// it does. If it works like you have it oh master though, then that's cool.

This comment has been minimized.

Copy link
@ryanml

ryanml Jul 11, 2019

Author Member

Updated this to chrome://wallet/home.html, seems that you don't get the brave page items without it, as you said.

One thing to keep in mind is that MM does use the relative path to open windows and views, so this may be something to go through and update across the board as well.

@ryanml ryanml force-pushed the no-init-notification branch from 1e4949b to 2bf2baa Jul 11, 2019
@ryanml ryanml merged commit 9b6b9d7 into master Jul 11, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@ryanml ryanml deleted the no-init-notification branch Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.