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

UX: Added Account list pinning and Unpinning #21865

Merged
merged 31 commits into from
Dec 13, 2023
Merged

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Nov 16, 2023

This PR is to add accounts pinning in the account list menu

Description

List management for accounts has been one of the most requested features from the community. In this iteration, we'll implement account list management through pin and unpinning accounts.

Related issues

Fixes: #20918

Manual testing steps

  1. Run the extension with NETWORK_ACCOUNT_DND=1 yarn start command
  2. Go to account list menu
  3. Click on the options
  4. Click on Pin Account to Top
  5. See the pinned account is on top
  6. Click on Pin Account for a new account, this account should be on second position from top
  7. Unpin Accounts

Screenshots/Recordings

Before

Screenshot 2023-11-17 at 1 11 16 PM

After

Screen.Recording.2023-11-17.at.7.25.07.PM.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@NidhiKJha NidhiKJha added DO-NOT-MERGE Pull requests that should not be merged team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Nov 16, 2023
@NidhiKJha NidhiKJha changed the title (WIP) account pinning UX: Account list pinning Nov 17, 2023
@NidhiKJha NidhiKJha changed the title UX: Account list pinning UX: Added Account list pinning and Unpinning Nov 17, 2023
@NidhiKJha NidhiKJha added team-extension-client and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 17, 2023
@NidhiKJha NidhiKJha marked this pull request as ready for review November 17, 2023 09:26
@NidhiKJha NidhiKJha requested a review from a team as a code owner November 17, 2023 09:26
DDDDDanica
DDDDDanica previously approved these changes Nov 21, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [08581fc]
Page Load Metrics (1350 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89146111168
domContentLoaded10261742
load10241633135013866
domInteractive10261742
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.42 KiB (0.04%)
  • ui: 664 Bytes (0.01%)
  • common: 102 Bytes (0.00%)

@k-g-j
Copy link
Contributor

k-g-j commented Dec 4, 2023

Pinning worked well but the expected behavior of when you pin a second account to top did not function as described. The second account pinned to top replaced the first pinned account as the top-most pin. I believe this may be due to account numbering. If a new pinned account has a lower account number than the top pinned account it replaces it on top and if its number is higher, it is pinned below. See attached videos:

Screen.Recording.2023-12-04.at.11.45.31.AM.mov
Screen.Recording.2023-12-04.at.11.51.32.AM.mov

@NidhiKJha
Copy link
Member Author

Pinning worked well but the expected behavior of when you pin a second account to top did not function as described. The second account pinned to top replaced the first pinned account as the top-most pin. I believe this may be due to account

@k-g-j @owencraston Nice shout, Have updated the logic here, also tried to address your comments in this commit @owencraston ceefd58. Also, the logic is once a new account is pinned, it will go in the bottom of the list. Example if account 1 is pinned and then account 5 is pinned and then account 3 is pinned, the list would look like:

account1
Account 5
account 3

then unpinned accounts.

Please refer to this screencast.

Screen.Recording.2023-12-05.at.2.26.38.PM.mov

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (2023178) 67.71% compared to head (b9cfbc9) 67.68%.
Report is 1 commits behind head on develop.

Files Patch % Lines
...n/account-list-item-menu/account-list-item-menu.js 23.53% 13 Missing ⚠️
app/scripts/metamask-controller.js 50.00% 6 Missing ⚠️
app/scripts/controllers/account-order.ts 63.64% 4 Missing ⚠️
ui/store/actions.ts 0.00% 3 Missing ⚠️
.../multichain/account-list-menu/account-list-menu.js 75.00% 2 Missing ⚠️
.../multichain/account-list-item/account-list-item.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #21865      +/-   ##
===========================================
- Coverage    67.71%   67.68%   -0.03%     
===========================================
  Files         1061     1062       +1     
  Lines        41143    41214      +71     
  Branches     11028    11053      +25     
===========================================
+ Hits         27859    27895      +36     
- Misses       13284    13319      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [b9cfbc9]
Page Load Metrics (1474 ± 146 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint924001887837
domContentLoaded10199606129
load99720381474305146
domInteractive10199606129
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.42 KiB (0.05%)
  • ui: 758 Bytes (0.01%)
  • common: 102 Bytes (0.00%)

@NidhiKJha NidhiKJha merged commit ce98650 into develop Dec 13, 2023
67 checks passed
@NidhiKJha NidhiKJha deleted the fix-account-pinning-20918 branch December 13, 2023 06:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@metamaskbot metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.9.0 Issue or pull request that will be included in release 11.9.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account list pinning
7 participants