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

fix: multi permisison dialogs for the same permission #442

Merged
merged 2 commits into from
Apr 6, 2018

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Apr 6, 2018

Fixes https://youtu.be/k4TMpAXToJc

Permission requests are asynchronous so it's possible for a user to request the same permission multiple times before a response is received. This PR deals with this case.

You can use the following code to check this PR is working:

Promise.all([ipfs.id(), ipfs.id()]).then(console.log).catch(console.log)

You should see 2 dialogs in master, and 1 dialog in this branch.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you opening this!
Indeed, dialog for every requested API method should appear only once.

Proposed fix works fine for multiple calls of the same API endpoint, but it gets tricky when you add another one:

Promise.all([ipfs.id(), ipfs.files.ls(), ipfs.id(), ipfs.files.ls(), ipfs.files.ls()]).then(console.log).catch(console.log)

Above snippet results in broken dialog for ipfs.files:

screenshot_1

This edge case needs to be fixed before we merge this.

@alanshaw
Copy link
Member Author

alanshaw commented Apr 6, 2018

@lidel is the issue that the second (and other) dialogs appear blank?

@lidel
Copy link
Member

lidel commented Apr 6, 2018

Yeah, it looks 'dead' which is bad UX.

@alanshaw
Copy link
Member Author

alanshaw commented Apr 6, 2018

@lidel fix for the issue is here: #443

@lidel
Copy link
Member

lidel commented Apr 6, 2018

@alanshaw perfect, tested together with #443 works as expected 👍 will merge this shortly.

@alanshaw
Copy link
Member Author

alanshaw commented Apr 6, 2018

🤦‍♂️ sorry, always with the hats

@lidel lidel merged commit 53ed839 into ipfs:master Apr 6, 2018
@alanshaw alanshaw deleted the fix/multi-dialog branch April 6, 2018 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants