Related to task T2132. Added logic to renameWallet in useWallet.js that checks if the wallet to be renamed is the currently active wallet, sets newWalletName for the currently active wallet. I did not include the delete wallet button for the currently active wallet, but this can be added if it is desired.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABC2086478395e9: [Cashtab] Add ability to rename active wallet
cd web/cashtab && npm start
navigate to the Configure screen
note that the saved wallets collapse is present even if there is only one saved wallet
Note that the currently active wallet now shows the add to address button & the edit name button
Edit the name of the currently active wallet
Observe that the active wallet's name has changed
From the wallet label component in Send, Airdrop, or Home screen, click the edit button,
Observe that the browser is routed to the Configure screen with the RenameWalletModal open,
Change the wallet name,
Observe that the wallet's name changes in the SW in local storage, the SW list on the configure screen, and in the wallet label component.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- rename-current-wallet
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19765 Build 39249: Build Diff cashtab-tests Build 39248: arc lint + arc unit
Event Timeline
| web/cashtab/src/components/Configure/Configure.js | ||
|---|---|---|
| 1586 | why is wallet necessary now when it was not previously? | |
| web/cashtab/src/hooks/useWallet.js | ||
| 783 | Consider the impact this will have if the user is changing the name of a saved wallet 🚨 You probably need a specific function for changing the name of the active wallet. It could be a simple wrapper, i.e. it could change wallet.name and also call renameWallet, which will update the data in the savedWallets object, which is necessary | |
Comment Actions
Changed logic in renameWallet to account for the activeWallet's name being changed, the active wallet & the copy of the activeWallet in the SW list are updated with this new logic.
Comment Actions
- Rename the exisitng renameWallet function to renameSavedWallet
- Create a new function renameActiveWallet in useWallet.js
- Pass wallet to this function as a parameter (along with oldName and newName)
- The new renameActiveWallet function should:
- Verify that no savedWallet has the same name, just like renameSavedWallet does
- Update savedWallets[i].name just like renameSavedWallet does
- Also update wallet.name and save to localforage
- setWallet(wallet) -- this will update the context of the active wallet
In addition to this, we will also want the 'edit' icon next to the wallet name on all screens, not just on the settings page. So, we don't need to change any logic about when the savedWallets dropdown appears.
| web/cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 749 ↗ | (On Diff #34744) | Good practice is to keep a function isolated to its parameters. Here, we are pulling in wallet from context. This is "the same place" that Configure.js grabs it from, but it would be cleaner to deliver wallet as a parameter to this function. At this point, though, I think we are creating enough complication in the difference between the logic of renaming a saved wallet and renaming the active wallet that there should be two distinct functions. |
Comment Actions
need to add edit button to the wallet name throughout the app to maintain SW list rendering logic on the configure screen.
Comment Actions
Added edit button to WalletLabel component which redirects to /configure and triggers the rename wallet modal.
Comment Actions
Good work. Well thought through diff that understands the moving parts and solves the problem. A few clean-up items in the inline comments.
I'm seeing this error in the dev console when I follow these steps.
- npm start
- Click the 'edit' icon on the home page
- See this error in the dev console
| web/cashtab/src/components/Common/WalletLabel.js | ||
|---|---|---|
| 4 ↗ | (On Diff #34775) | This should be done in CustomIcons.js Update: I see this is how it's currently imported into Configure.js, along with some others Please create a task to move all icon imports like this into CustomIcons.js and assign it to yourself. Don't fix it in this diff. |
| web/cashtab/src/components/Configure/Configure.js | ||
| 796 ↗ | (On Diff #34775) | We shouldn't have two if blocks with mutually exclusive conditions. That's what an else {} block is for. |
Comment Actions
Updated snapshots & rebased to master. Fixed console error, imported Edit icon from CustomIcons and refactored changeWalletName per review feedback.