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

Implement edit/remove networks/accounts/keys. #77

Merged
merged 15 commits into from
Sep 16, 2024
Merged

Conversation

brusherru
Copy link
Member

It closes #42, closes #43, closes #44.

I had to refactor some input components (related to key and account selection).
So please, check it out carefully, just in case I broke something and didn't find :)
Actually more refactoring is needed, but I decided to finish it later.

Also, I moved adding of the new network inside the special "Edit networks" drawer (mb better to rename it into "Manage networks"?). Hopefully it's fine from the UX perspective.

Copy link

github-actions bot commented Sep 6, 2024

You can preview the changes at : https://ca5d83e0.smapp-lite-prod.pages.dev

Copy link
Collaborator

@monikasmolarek monikasmolarek left a comment

Choose a reason for hiding this comment

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

Also, login page got broken - the incorrect password msg doesn’t show up, the button freezes on unlocking.

Screen.Recording.2024-09-06.at.11.38.49.mov

src/components/EditNetworksDrawer.tsx Outdated Show resolved Hide resolved
src/components/EditNetworksDrawer.tsx Outdated Show resolved Hide resolved
src/components/KeyManager.tsx Outdated Show resolved Hide resolved
src/components/KeyManager.tsx Outdated Show resolved Hide resolved
src/components/KeyManager.tsx Show resolved Hide resolved
src/components/KeyManager.tsx Show resolved Hide resolved
Copy link
Collaborator

@monikasmolarek monikasmolarek left a comment

Choose a reason for hiding this comment

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

For the UX part:

Once the user deletes all accounts, there's this screen suggesting to switch the account.
Screenshot 2024-09-06 at 11 08 47

Sometime in the future, it would be good to improve it - show msg that there are no accounts currently available, instruct to go create/import one, maybe even include a link opening the create or/and import account.

Copy link

You can preview the changes at : https://765f10ab.smapp-lite-prod.pages.dev

@brusherru
Copy link
Member Author

brusherru commented Sep 10, 2024

@monikasmolarek thanks a lot!
I have:

  1. updated texts,
  2. fixed these two bugs (when no keys / accounts),
  3. tweaked GUI for the case when there is no account in the wallet.
image

Also I can confirm that there is a bug as you recorded on the video above.
I checked that we have the same bug in the main branch.
So I will fix it within a separate PR.
Created an issue for it: #79

Copy link
Collaborator

@monikasmolarek monikasmolarek left a comment

Choose a reason for hiding this comment

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

just noticed one more tiny detail - the modal to delete the account has a button that says "Delete Key".
Nothing serious but it actually confused me a bit while doing things quickly and I had to cancel and start again to make sure I was not mistaken and deleted account, not key.

Screenshot 2024-09-10 at 09 56 24

@monikasmolarek
Copy link
Collaborator

Also, the current possibilities open a new use case.

For example: What if someone deletes both - all accounts, all keys -> The wallet is empty, we can of course create a new account - derive it from a foreign key, but the modal still has the "local key" drop-down, which is now empty. Should we allow to put there a hex and inform that it imports this key automatically while creating the account? Or should we hide the local key option?
WDYT? Worth creating a feature request?
Screenshot 2024-09-10 at 10 01 23

The transactions' modals/management handle view-only perfectly well, not mentioning the local key to sign when the key isn't there. 👌 So I thought managing accounts could also be more robust.

@brusherru
Copy link
Member Author

brusherru commented Sep 11, 2024

Should we allow to put there a hex and inform that it imports this key automatically while creating the account? Or should we hide the local key option?

It allows to put there a public key, but you need to switch to the foreign key, yeah.
However, this won't be an issue when #82 will be implemented: Local Key dropdown will always have "Create a new key" option. So I think we don't need a separate issue for it.

Copy link

You can preview the changes at : https://7eb429d1.smapp-lite-prod.pages.dev

@dioexul
Copy link

dioexul commented Sep 12, 2024

Screenshot 2024-09-11 at 21 30 55 Do we need "templateAddress" field here?

Also for me it is strange that I can replace the key for an existing account.

Copy link

You can preview the changes at : https://fbeb1312.smapp-lite-prod.pages.dev

@brusherru
Copy link
Member Author

@dioexul I've added a yellow text under the name input saying that changing anything below will affect the account's address and change it only on your own risk. I think now it will be more clear that in a common case you should not change anything. But if you really have a mistake there — it will be possible to change anything.

Sounds good?

@dioexul
Copy link

dioexul commented Sep 15, 2024

@brusherru yes, having a warning is better.
I'm still thinking do we really need this functionality for all users, for developers - we definitely need it.

Copy link

You can preview the changes at : https://19b5255a.smapp-lite-prod.pages.dev

@brusherru brusherru merged commit 58cbf4d into main Sep 16, 2024
1 check passed
@brusherru brusherru deleted the feat-edit-data branch September 16, 2024 15:26
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.

Edit/Delete Accounts Rename/Delete Keys Edit/Delete Networks
3 participants