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

ui: Settings screen according to figma #296

Merged
merged 12 commits into from
Jun 7, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented May 26, 2022

Adapt Settings screen according to Figma.

Differences:

  • "Log out" is in section "Wallet" instead of "Settings"

Question:

📸 Before

📸 After

@theborakompanioni theborakompanioni added the UI/UX Issue related to cosmetics, design, or user experience label May 26, 2022
@theborakompanioni theborakompanioni self-assigned this May 26, 2022
@theborakompanioni theborakompanioni force-pushed the settings-screen-figma branch 2 times, most recently from 4101616 to 69d2541 Compare May 26, 2022 15:45
@editwentyone
Copy link

editwentyone commented May 26, 2022

the Logout is for the whole app, not on wallet level. it should go over Wallet category please :)

maybe even "Log out from Jam"

@theborakompanioni
Copy link
Collaborator Author

maybe even "Log out from Jam"

Hmm.. I am not sure I know what this means.

Currently "logging out" is just forgetting the session key. What should happen, when someone "logs out"?

@editwentyone
Copy link

its should bring you to "login page" https://www.figma.com/file/kfejZJFlwBywvLEnPEmJo1/?node-id=3994%3A73345

but I think we do not have that yet

@theborakompanioni theborakompanioni changed the title ui Settings screen according to figma ui: Settings screen according to figma May 27, 2022
@theborakompanioni theborakompanioni requested a review from a user May 30, 2022 07:43
@ghost
Copy link

ghost commented Jun 2, 2022

the Logout is for the whole app, not on wallet level.

I think these are equivalent things at this time (as @theborakompanioni mentioned) right?

Edit: You are right, in the future when we have a "single wallet flow" and a central, wallet-independent login page, it should go there of course.

@@ -1,5 +1,6 @@
export const routes = {
home: '/',
Copy link

Choose a reason for hiding this comment

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

Is home still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought about this myself. Opted for keeping it, as e.g. the navbar does not need to know what the actual route is (and using "/" hardcoded for this feels inconsistent). Is this reasonable?

Copy link

@ghost ghost Jun 3, 2022

Choose a reason for hiding this comment

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

No but I mean routes.home and routes.walletList are duplicates, right?

@editwentyone
Copy link

editwentyone commented Jun 3, 2022

I think that "log out" from Wallet, because its now used differently, is misleading.

logout should be only used when the user logged in somewhere, if that's not the case, we need to rethink what's actually happening.

I would move logout back up again and hide it as long as we don't have login implemented. why would a user logout aka "lock" the wallet and not just close the window? what happens when we just close the window?

~

so we have:

  • create wallet: creating new wallets, not possible while another wallet is still used and unlocked
  • unlock wallet: wallet is inactive, needs password to be active
  • open wallet: entering, using, wallet is active
  • lock wallet: deactivate wallet, wallet is now inactive
  • Logout wallet (misunderstanding): wallet stays active in the background, needs password to be opened/ entered again

@theborakompanioni
Copy link
Collaborator Author

I would move logout back up again and hide it as long as we don't have login implemented.

Removed it for now.

why would a user logout aka "lock" the wallet and not just close the window?

You are right, the (now removed) "Log out" action, by forgetting the auth token, would have been the same as closing the window.
However, I can relate to some uneasiness when closing a tab with an "unlocked" wallet. A dedicated button would "feel" a little better, albeit there is no difference in what actually happens.

what happens when we just close the window?

The auth token is removed (as sessionStorage cleared when the page session ends).

~

so we have:

* **create wallet**: creating new wallets, not possible while another wallet is still used and unlocked

* **unlock wallet**: wallet is _inactive_, **needs password** to be active

* **open wallet**: entering, using, wallet is _active_

* **lock wallet**: deactivate wallet, wallet is now _inactive_

* **Logout** wallet (misunderstanding): wallet stays active in the background, **needs password** to be opened/ entered again

Maybe something like "Leave wallet", "Exit wallet" or "Close wallet" (although the term "Closing" might be confused by "Locking")?

In any case, I have just removed it for now and will support anything that you think makes most sense from an UI/UX perspective.
Do you think these changes can be merged?

@ghost
Copy link

ghost commented Jun 6, 2022

Removing seems like n acceptable solution for now. 👍 All the nuances are way to complicated, we really need to figure out a good concept for this.


A little padding for the buttons on mobile would be great.

@theborakompanioni
Copy link
Collaborator Author

A little padding for the buttons on mobile would be great.

Yeah, did not change that, as I supposed it was intentional. Any improvements are very welcome! Feel free to just push to this branch!

@ghost
Copy link

ghost commented Jun 6, 2022

Yeah, did not change that

Good point! :D Will push an update.

Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

hidden is ok for me, I sketched already something for the next call, and yeah: its confusing :P

@0xSaksham
Copy link
Contributor

0xSaksham commented Aug 8, 2024

@theborakompanioni, I was taking inspiration for ConfirmModal use case and found out that this feature doesn't work here. If you want I can work on it?

@theborakompanioni
Copy link
Collaborator Author

@theborakompanioni, I was taking inspiration for ConfirmModal use case and found out that this feature doesn't work here. If you want I can work on it?

I am not quite sure what you are referring to.. can you elaborate?

@0xSaksham
Copy link
Contributor

Confirm modal wants you to confirm that you really want to do this.

But there's no modal opening up here.

@theborakompanioni
Copy link
Collaborator Author

Sorry @0xSaksham, I still do not understand. What is it you want to work on or which change do you have in mind in the context of this issue?

@0xSaksham
Copy link
Contributor

@theborakompanioni let's discuss this in tomorrow's meeting.

If there's no meeting, we'll discuss this in one-on-one call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants