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: wallet-ui multiple accounts scrollbar #499

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

khanti42
Copy link
Collaborator

This PR adds the support of infinite account to Wallet-UI

Changes :

  • The "My Account" is replaced by "Account X" where is X is the account number (accountIndex + 1)
  • The list of accounts now uses the Account Icon and Account name and short address for better UI/UX
  • When the account number is higher than 3 a scrollbar appears

See the video below :

Screen.Recording.2025-01-28.at.15.08.41.mov

@khanti42 khanti42 requested a review from a team as a code owner January 28, 2025 14:12
@khanti42 khanti42 requested review from Akaryatrh and wantedsystem and removed request for a team January 28, 2025 14:12
@khanti42 khanti42 changed the title feat: [Wallet-UI] multiple accounts scrollbar feat: wallet-ui multiple accounts scrollbar Jan 30, 2025
Copy link
Collaborator

@stanleyyconsensys stanleyyconsensys left a comment

Choose a reason for hiding this comment

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

Left some comments, and there are 2 important comments/questions here

  1. Didnt we agree to use modal , rather than pull out to support scroll bar? as. modal can be more responsive, but im okay with that too (Network poll up is different, it only have 2, not dynamic)

  2. Address index is a requirement for next ticket (https://consensyssoftware.atlassian.net/browse/SF-785), what is the reason to have this in this ticket? as the user story has not mention this (https://consensyssoftware.atlassian.net/jira/software/projects/SF/boards/472?selectedIssue=SF-784)

packages/wallet-ui/src/slices/walletSlice.ts Outdated Show resolved Hide resolved
packages/wallet-ui/src/slices/walletSlice.ts Outdated Show resolved Hide resolved
@khanti42
Copy link
Collaborator Author

khanti42 commented Feb 3, 2025

The reason to use index here already, is to show Account Number in the list. We could have a separate index but as we already have it from backend I thought it would make sense to add it here already.

Regarding the modal. I felt that the UX/UI was good. The switch appears closer to the button so it's less extra movement for the mouse and this feels more like an integrated approach. Except if there is a strong no go on that I would suggest to keep it this way. Maybe we can revisit this when integrating design change to include dApps listing..

Copy link
Collaborator

@stanleyyconsensys stanleyyconsensys left a comment

Choose a reason for hiding this comment

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

Left 2 comments,

And there is a recommend comment
i can see some component retrieve state from redux, but at the meanwhile it retrieve the redux state from parent

if you dont mind, i will suggest, to align to use redux
the redux purpose is to avoid pass state from each level

as i already seen passing state from
home -> sidebar -> account detail / account switch

in general a component only stateless if it is a re-useable component , such as AddressInput, AmountInput

i know it may have a lot of change if we do that

I would suggest lets move that slowly, at least

  • AccountSwitchModalView -> keep full , and starkName
  • SideBarView - no props
  • AccountDetailsModal - no props

those can be stateful without the need passing address / addressIndex

in additional, i think you can also make the currentAccount from redux state to have a dummy object to include the dummy address, so when changing SideBarView, you dont need to much effort

WDYT

Copy link

sonarqubecloud bot commented Feb 4, 2025

Quality Gate Passed Quality Gate passed for 'consensys_starknet-snap-wallet-ui'

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.3% Duplication on New Code

See analysis details on SonarQube Cloud

Copy link

sonarqubecloud bot commented Feb 4, 2025

Quality Gate Passed Quality Gate passed for 'consensys_starknet-snap-starknet-snap'

Issues
17 New issues
0 Accepted issues

Measures
0 Security Hotspots
1.8% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarQube Cloud

@khanti42
Copy link
Collaborator Author

khanti42 commented Feb 4, 2025

Regarding this comment :

in additional, i think you can also make the currentAccount from redux state to have a dummy object to include the dummy address, so when changing SideBarView, you dont need to much effort

Its solved in an other PR: #502

Copy link
Collaborator

@stanleyyconsensys stanleyyconsensys left a comment

Choose a reason for hiding this comment

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

LGTM

@stanleyyconsensys stanleyyconsensys merged commit ef9e9cf into feat/enable-multiple-accounts Feb 5, 2025
14 checks passed
@stanleyyconsensys stanleyyconsensys deleted the feat/sf-796 branch February 5, 2025 03:36
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