-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: update notification account settings to use BIP-44 designs #20307
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: update notification account settings to use BIP-44 designs #20307
Conversation
we still need to update tests and styles, which I'll get to later
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…S-1260/notification-account-settings-bip-44-components
…S-1260/notification-account-settings-bip-44-components
|
This comment was marked as spam.
This comment was marked as spam.
|
app/components/Views/Settings/NotificationsSettings/AccountsList.test.tsx
Show resolved
Hide resolved
|
app/component-library/components-temp/MultichainAccounts/AccountCell/AccountCell.tsx
Outdated
Show resolved
Hide resolved
…260/notification-account-settings-bip-44-components
…260/notification-account-settings-bip-44-components
…260/notification-account-settings-bip-44-components
…260/notification-account-settings-bip-44-components
…260/notification-account-settings-bip-44-components
fbf39c0
5511395 to
fbf39c0
Compare
|
Had to force-push since this branch was pretty out of date (and the pre-commit to merge hooks were failing). |
app/components/Views/Settings/NotificationsSettings/AccountsList.test.tsx
Show resolved
Hide resolved
after merging main, some places use other selectors, might as well just provide mock state to our tests
|
| color={TextColor.Alternative} | ||
| /> | ||
| </TouchableOpacity> | ||
| {endContainer || ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified so we can pass in either a custom component or rely on the existing component. Provides more flexibility.
...emp/MultichainAccounts/MultichainAccountSelectorList/AccountListHeader/AccountListHeader.tsx
Show resolved
Hide resolved
app/components/Views/Settings/NotificationsSettings/AccountsList.hooks.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for accounts owned files (not tested)
| color={TextColor.Alternative} | ||
| /> | ||
| </TouchableOpacity> | ||
| {endContainer || ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I guess ?? would have been a bit more appropriate, but I'm ok with the || too 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha good callout. Emm might visit this later since I have approvals 🤪
I've tested this feature with the team 👍🏾
vinnyhoward
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



Description
This reuses the BIP-44 designs for the notification account settings.
I also took this as an opportunity to improve the style and spacing for the notification settings view.
NOTE - once we support multi-SRP notifications, we will probably clean up this settings page. Potentially remove the per account notification settings (notifications will be on all accounts or no accounts)
Changelog
CHANGELOG entry: refactor: cleanup notification account settings to use BIP-44 designs.
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-1260
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Refactors Notifications settings to use multichain account groups (BIP-44) with AccountCell UI, updates hooks to operate on account IDs/EVM addresses, and adjusts styles and tests/e2e.
AccountCelland section headerAccountListHeader.endContainersupport inAccountCellwithBalanceEndContainerfor balance/menu; used to render per-account switches.NotificationsSettings.styles) to add separators, spacing tweaks, and header layout.useNotificationAccountListPropsnow derives addresses from account IDs, normalizes EVM addresses, and exposesgetEvmAddressplus ID-basedisAccountLoading/isAccountEnabled.useFirstHDWalletAccountsand updateuseAccountPropsto returnfirstHDWalletGroups+ avatar type.AccountsListnow listsfirstHDWalletGroups.data, rendersNotificationOptionTogglewith group item and computed EVM address, and shows a section header.NotificationOptionTogglecomposesAccountCelland renders loading/switch viaendContainer.AccountListHeaderaccepts optionalcontainerStyle.Written by Cursor Bugbot for commit 0e45afd. This will update automatically on new commits. Configure here.