Skip to content

Commit

Permalink
UX: Added Account list pinning and Unpinning (#21865)
Browse files Browse the repository at this point in the history
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](https://docs.google.com/presentation/d/1mpgU2Zixmn6bb8Y3LPMFRUCQxFiEbYwGEWuzNx8jvBo/edit#slide=id.g2588819e87c_6_0)
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](https://github.com/MetaMask/metamask-extension/assets/39872794/0fa9bbe1-c614-44a3-a0a2-14bc0976dc9a)


### **After**




https://github.com/MetaMask/metamask-extension/assets/39872794/abf07bf3-9baf-49d4-b27e-20ed3504d493





## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.
  • Loading branch information
NidhiKJha authored Dec 13, 2023
1 parent dd00145 commit ce98650
Show file tree
Hide file tree
Showing 23 changed files with 336 additions and 10 deletions.
1 change: 1 addition & 0 deletions .storybook/test-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const state = {
},
},
orderedNetworkList: [],
pinnedAccountList: [],
tokenList: {
'0x514910771af9ca656af840dff83e8264ecf986ca': {
address: '0x514910771af9ca656af840dff83e8264ecf986ca',
Expand Down
6 changes: 6 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions app/images/icons/pin.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions app/images/icons/unpin.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
96 changes: 96 additions & 0 deletions app/scripts/controllers/account-order.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import {
BaseControllerV2,
RestrictedControllerMessenger,
} from '@metamask/base-controller';

// Unique name for the controller
const controllerName = 'AccountOrderController';

/**
* The account ID of a account.
*/
export type AccountAddress = string;

// State shape for AccountOrderController
export type AccountOrderControllerState = {
pinnedAccountList: AccountAddress[];
};

// Describes the action for updating the accounts list
export type AccountOrderControllerupdateAccountsListAction = {
type: `${typeof controllerName}:updateAccountsList`;
handler: AccountOrderController['updateAccountsList'];
};

// Union of all possible actions for the messenger
export type AccountOrderControllerMessengerActions =
AccountOrderControllerupdateAccountsListAction;

// Type for the messenger of AccountOrderController
export type AccountOrderControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
AccountOrderControllerMessengerActions,
never,
never,
never
>;

// Default state for the controller
const defaultState = {
pinnedAccountList: [],
};

// Metadata for the controller state
const metadata = {
pinnedAccountList: {
persist: true,
anonymous: true,
},
};

/**
* Controller that updates the order of the account list.
* This controller subscribes to account state changes and ensures
* that the account list is updated based on the latest account configurations.
*/
export class AccountOrderController extends BaseControllerV2<
typeof controllerName,
AccountOrderControllerState,
AccountOrderControllerMessenger
> {
/**
* Creates a AccountOrderController instance.
*
* @param args - The arguments to this function.
* @param args.messenger - Messenger used to communicate with BaseV2 controller.
* @param args.state - Initial state to set on this controller.
*/
constructor({
messenger,
state,
}: {
messenger: AccountOrderControllerMessenger;
state?: AccountOrderControllerState;
}) {
// Call the constructor of BaseControllerV2
super({
messenger,
metadata,
name: controllerName,
state: { ...defaultState, ...state },
});
}

/**
* Updates the accounts list in the state with the provided list of accounts.
*
* @param accountList - The list of accounts to update in the state.
*/

updateAccountsList(accountList: []) {
this.update((state) => {
state.pinnedAccountList = accountList;
return state;
});
}
}
3 changes: 3 additions & 0 deletions app/scripts/lib/setupSentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export const SENTRY_BACKGROUND_STATE = {
NetworkOrderController: {
orderedNetworkList: [],
},
AccountOrderController: {
pinnedAccountList: [],
},
AppMetadataController: {
currentAppVersion: true,
currentMigrationVersion: true,
Expand Down
22 changes: 22 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ import {
import createOriginMiddleware from './lib/createOriginMiddleware';
import createTabIdMiddleware from './lib/createTabIdMiddleware';
import { NetworkOrderController } from './controllers/network-order';
import { AccountOrderController } from './controllers/account-order';
import createOnboardingMiddleware from './lib/createOnboardingMiddleware';
import { setupMultiplex } from './lib/stream-utils';
import EnsController from './controllers/ens';
Expand Down Expand Up @@ -842,6 +843,15 @@ export default class MetamaskController extends EventEmitter {
messenger: networkOrderMessenger,
state: initState.NetworkOrderController,
});

const accountOrderMessenger = this.controllerMessenger.getRestricted({
name: 'AccountOrderController',
});
this.accountOrderController = new AccountOrderController({
messenger: accountOrderMessenger,
state: initState.AccountOrderController,
});

// token exchange rate tracker
this.tokenRatesController = new TokenRatesController(
{
Expand Down Expand Up @@ -1936,6 +1946,7 @@ export default class MetamaskController extends EventEmitter {
SubjectMetadataController: this.subjectMetadataController,
AnnouncementController: this.announcementController,
NetworkOrderController: this.networkOrderController,
AccountOrderController: this.accountOrderController,
GasFeeController: this.gasFeeController,
TokenListController: this.tokenListController,
TokensController: this.tokensController,
Expand Down Expand Up @@ -1988,6 +1999,7 @@ export default class MetamaskController extends EventEmitter {
SubjectMetadataController: this.subjectMetadataController,
AnnouncementController: this.announcementController,
NetworkOrderController: this.networkOrderController,
AccountOrderController: this.accountOrderController,
GasFeeController: this.gasFeeController,
TokenListController: this.tokenListController,
TokensController: this.tokensController,
Expand Down Expand Up @@ -3174,6 +3186,7 @@ export default class MetamaskController extends EventEmitter {
markNotificationsAsRead: this.markNotificationsAsRead.bind(this),
updateCaveat: this.updateCaveat.bind(this),
updateNetworksList: this.updateNetworksList.bind(this),
updateAccountsList: this.updateAccountsList.bind(this),
getPhishingResult: async (website) => {
await phishingController.maybeUpdateState();

Expand Down Expand Up @@ -5504,6 +5517,15 @@ export default class MetamaskController extends EventEmitter {
}
};

updateAccountsList = (pinnedAccountList) => {
try {
this.accountOrderController.updateAccountsList(pinnedAccountList);
} catch (err) {
log.error(err.message);
throw err;
}
};

rejectPermissionsRequest = (requestId) => {
try {
this.permissionController.rejectPermissionsRequest(requestId);
Expand Down
1 change: 0 additions & 1 deletion builds.yml
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,6 @@ env:
# Determines if feature flagged Multichain UI should be used
- MULTICHAIN: ''
- NETWORK_ACCOUNT_DND: ''

###
# Meta variables
###
Expand Down
1 change: 1 addition & 0 deletions test/data/mock-state.json
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@
"useNftDetection": true,
"openSeaEnabled": true,
"orderedNetworkList": [],
"pinnedAccountList": [],
"advancedGasFee": {
"0x5": {
"maxBaseFee": "75",
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/fixture-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ function defaultFixture() {
NetworkOrderController: {
orderedNetworkList: [],
},
AccountOrderController: {
pinnedAccountList: [],
},
AppStateController: {
browserEnvironment: {},
nftsDropdownState: {},
Expand Down Expand Up @@ -487,6 +490,11 @@ class FixtureBuilder {
return this;
}

withAccountOrderController(data) {
merge(this.fixture.data.AccountOrderController, data);
return this;
}

withAppStateController(data) {
merge(this.fixture.data.AppStateController, data);
return this;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{
"AccountOrderController": { "pinnedAccountList": {} },
"AccountTracker": {
"accounts": "object",
"accountsByChainId": "object"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
"subjectMetadata": "object",
"announcements": "object",
"orderedNetworkList": { "0": "string", "1": "string", "2": "string" },
"pinnedAccountList": {},
"gasFeeEstimates": {},
"gasFeeEstimatesByChainId": {},
"estimatedGasFeeTimeBounds": {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
},
"AnnouncementController": { "announcements": "object" },
"NetworkOrderController": { "orderedNetworkList": {} },
"AccountOrderController": { "pinnedAccountList": {} },
"AppStateController": {
"browserEnvironment": {},
"nftsDropdownState": {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
},
"AnnouncementController": { "announcements": "object" },
"NetworkOrderController": { "orderedNetworkList": {} },
"AccountOrderController": { "pinnedAccountList": {} },
"AppStateController": {
"browserEnvironment": {},
"nftsDropdownState": {},
Expand Down
2 changes: 2 additions & 0 deletions ui/components/component-library/icon/icon.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export enum IconName {
Notification = 'notification',
PasswordCheck = 'password-check',
People = 'people',
Pin = 'pin',
ProgrammingArrows = 'programming-arrows',
Custody = 'custody',
Question = 'question',
Expand Down Expand Up @@ -162,6 +163,7 @@ export enum IconName {
Twitter = 'twitter',
QrCode = 'qr-code',
UserCheck = 'user-check',
Unpin = 'unpin',
Ban = 'ban',
Bold = 'bold',
CircleX = 'circle-x',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getCurrentChainId,
getHardwareWalletType,
getAccountTypeForKeyring,
getPinnedAccountsList,
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
getMetaMaskAccountsOrdered,
///: END:ONLY_INCLUDE_IF
Expand All @@ -31,7 +32,7 @@ import {
MetaMetricsEventCategory,
MetaMetricsEventName,
} from '../../../../shared/constants/metametrics';
import { showModal } from '../../../store/actions';
import { showModal, updateAccountsList } from '../../../store/actions';
import { TextVariant } from '../../../helpers/constants/design-system';
import { formatAccountType } from '../../../helpers/utils/metrics';
import { AccountDetailsMenuItem, ViewExplorerMenuItem } from '..';
Expand All @@ -45,6 +46,7 @@ export const AccountListItemMenu = ({
isRemovable,
identity,
isOpen,
isPinned,
}) => {
const t = useI18nContext();
const trackEvent = useContext(MetaMetricsContext);
Expand All @@ -59,6 +61,8 @@ export const AccountListItemMenu = ({
);
const accountType = formatAccountType(getAccountTypeForKeyring(keyring));

const pinnedAccountList = useSelector(getPinnedAccountsList);

///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
const isCustodial = keyring?.type ? /Custody/u.test(keyring.type) : false;
const accounts = useSelector(getMetaMaskAccountsOrdered);
Expand Down Expand Up @@ -121,6 +125,18 @@ export const AccountListItemMenu = ({
};
}, [handleClickOutside]);

const handlePinning = (address) => {
const updatedPinnedAccountList = [...pinnedAccountList, address];
dispatch(updateAccountsList(updatedPinnedAccountList));
};

const handleUnpinning = (address) => {
const updatedPinnedAccountList = pinnedAccountList.filter(
(item) => item !== address,
);
dispatch(updateAccountsList(updatedPinnedAccountList));
};

return (
<Popover
className="multichain-account-list-item-menu__popover"
Expand All @@ -147,6 +163,22 @@ export const AccountListItemMenu = ({
textProps={{ variant: TextVariant.bodySm }}
address={identity.address}
/>
{process.env.NETWORK_ACCOUNT_DND ? (
<MenuItem
data-testid="account-list-menu-pin"
onClick={() => {
isPinned
? handleUnpinning(identity.address)
: handlePinning(identity.address);
onClose();
}}
iconName={isPinned ? IconName.Unpin : IconName.Pin}
>
<Text variant={TextVariant.bodySm}>
{isPinned ? t('unpin') : t('pinToTop')}
</Text>
</MenuItem>
) : null}
{isRemovable ? (
<MenuItem
ref={removeAccountItemRef}
Expand Down Expand Up @@ -241,6 +273,10 @@ AccountListItemMenu.propTypes = {
* Represents if the account should be removable
*/
isRemovable: PropTypes.bool.isRequired,
/**
* Represents pinned accounts
*/
isPinned: PropTypes.bool,
/**
* Identity of the account
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ exports[`AccountListItem renders AccountListItem component and shows account nam
class="mm-box mm-box--display-flex mm-box--justify-content-space-between"
>
<div
class="mm-box multichain-account-list-item__account-name mm-box--margin-inline-end-2"
class="mm-box multichain-account-list-item__account-name mm-box--margin-inline-end-2 mm-box--display-flex mm-box--gap-2 mm-box--align-items-center"
>
<button
class="mm-box mm-text multichain-account-list-item__account-name__button mm-text--body-md-medium mm-text--ellipsis mm-text--text-align-left mm-box--padding-0 mm-box--width-full mm-box--color-text-default mm-box--background-color-transparent"
Expand Down
Loading

0 comments on commit ce98650

Please sign in to comment.