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

wallet-ext: disconnect dapp popup #4714

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

pchrysochoidis
Copy link
Contributor

Screen.Recording.2022-09-19.at.09.11.19.mov

closes #4696

@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2022

💳 Wallet Extension has been built, you can download the packaged extension here: https://github.com/MystenLabs/sui/actions/runs/3085205245#artifacts

@@ -76,6 +76,7 @@
},
"dependencies": {
"@mysten/sui.js": "workspace:*",
"@popperjs/core": "^2.11.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Popper has a successor called Floating UI, which I'd use if you can instead: https://floating-ui.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool looks fancy! I am not familiar and I have limited time today, do you think changing it in another PR would be ok? (I will create an issue to track it)

@@ -224,6 +224,16 @@ class Permissions {
);
}

public async clear(origin: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear kind of implies removing all, maybe call this delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-4696-apps-connected-indicator branch from 5bdfb7a to 7c2dc9c Compare September 19, 2022 16:38
Copy link
Collaborator

@mystie711 mystie711 left a comment

Choose a reason for hiding this comment

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

@pchrysochoidis don't have opinion's on the floater ui Jordan speaks of. It did remind me of the need to add a gentle animation to the popup that appears on tap in header.

Could we add a gentle (300ms) "fade-in + grow" animation for the the Disconnect ui on-click?

@pchrysochoidis
Copy link
Contributor Author

@pchrysochoidis don't have opinion's on the floater ui Jordan speaks of. It did remind me of the need to add a gentle animation to the popup that appears on tap in header.

Could we add a gentle (300ms) "fade-in + grow" animation for the the Disconnect ui on-click?

Yeah I remember our discussion but I think we decided that we can go without the animation first and add them as a next step. I will add an issue to track this.

Is this ok with you @mystie711? (Merge this without animations now and improve it with another PR)

Copy link
Contributor

@Jordan-Mysten Jordan-Mysten left a comment

Choose a reason for hiding this comment

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

Code LGTM. If you have time to add the animation, feel free to do so, but also feel free to punt to the next release if it'll take too long.

@pchrysochoidis
Copy link
Contributor Author

Code LGTM. If you have time to add the animation, feel free to do so, but also feel free to punt to the next release if it'll take too long.

I will postpone it to the next release, to make sure we have the feature in this release and continue with the improvements.

Copy link
Collaborator

@mystie711 mystie711 left a comment

Choose a reason for hiding this comment

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

Agreed to tackle animation in a separate task with @pchrysochoidis .

@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-4696-apps-connected-indicator branch from 7c2dc9c to f59265b Compare September 19, 2022 18:25
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-4696-apps-connected-indicator branch from f59265b to d41c6ef Compare September 19, 2022 19:49
@pchrysochoidis pchrysochoidis merged commit 021980e into main Sep 19, 2022
@pchrysochoidis pchrysochoidis deleted the pc-wallet-ext-4696-apps-connected-indicator branch September 19, 2022 20:22
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.

wallet-ext: apps connected indicator
3 participants