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

feat: change active account on dapp login #3454

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

alex-k8
Copy link
Collaborator

@alex-k8 alex-k8 commented Jan 20, 2025

closes: #3449
also fixes a UI issue on wallet connect popup's dimensions

@alex-k8 alex-k8 requested a review from CedrikNikita January 20, 2025 12:18
Copy link

default:
return 0;
}
});
const tokenSymbol = computed(
() => ProtocolAdapterFactory.getAdapter(account!.protocol).coinSymbol,
() => ProtocolAdapterFactory.getAdapter(account!.value.protocol).coinSymbol,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
() => ProtocolAdapterFactory.getAdapter(account!.value.protocol).coinSymbol,
() => ProtocolAdapterFactory.getAdapter(account.value.protocol).coinSymbol,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the unnecessary !

@@ -3,6 +3,9 @@
class="truncate"
:class="{ right }"
>
<span class="center-vertically">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you decided to add it inside Truncate component and not just part of AccountInfo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to AccountInfo as it's not necessary in Truncate

@@ -340,7 +339,7 @@ export default defineComponent({
position: relative;
margin: 0 auto;
width: 100%;
height: 100%;
height: 100vh;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for that change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was an issue when the extension opened as a popup (i.e. when logging in through a dapp or when signing transactions). The content of the popup that opened had a wrong height, which caused the buttons at the bottom to be cut. When that popup was resized, they disappeared altogether. The 100vh in combination with removing the justify-content: center; fixed this behavior. It did not seem to affect anything else according to my tests, but should be double checked.

Copy link
Collaborator

@CedrikNikita CedrikNikita Jan 21, 2025

Choose a reason for hiding this comment

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

#2208 Original change was done during moving to ionic
@dvdbak This should be tested with all the potential cases.

  1. Android since the original changes is related to Android i think we might spot the difference here
  2. Extension main popup
  3. Extension popup window
  4. Extension open as a new tab
  5. Web
  6. Iframe

@alex-k8 alex-k8 force-pushed the feat/change-active-account-on-dapp-login branch 2 times, most recently from 4f9a71f to 6f8a1e3 Compare January 20, 2025 15:59
Copy link

1 similar comment
Copy link

@@ -137,6 +121,7 @@ export default defineComponent({
showBookmarked,
setAccountSelectType,
} = useAccountSelector();
const { getAccountIcon } = useAccounts();
Copy link
Collaborator

Choose a reason for hiding this comment

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

getAccountIcon is not used in this component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

@alex-k8 alex-k8 force-pushed the feat/change-active-account-on-dapp-login branch 3 times, most recently from e1a50bf to 3324b06 Compare January 22, 2025 11:59
Copy link

1 similar comment
Copy link

@alex-k8 alex-k8 force-pushed the feat/change-active-account-on-dapp-login branch from 3324b06 to f408531 Compare January 24, 2025 10:19
Copy link

@alex-k8 alex-k8 force-pushed the feat/change-active-account-on-dapp-login branch from f408531 to 04dbacd Compare January 24, 2025 16:13
Copy link

@dvdbak
Copy link
Collaborator

dvdbak commented Jan 24, 2025

LGTM
Windows - Chrome ext, FF ext
Android

Copy link

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.

dapp Wallet Connection - Ability to change active account on Login
4 participants