-
Notifications
You must be signed in to change notification settings - Fork 256
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: new account management view #1172
Conversation
Looks really cool! Awesome actually! One thing I'd change is to only show the icon in the sidebar in case there are multiple accounts already, what do you think? |
Thanks mate - appreciate it🙇. Been a bit of a journey to get to this point, but it's all starting to fall into place nicely
I moved the |
One possible suggestion...we could decide to move the Account Management button into the Settings footer, in place of where the "add extra accounts" buttons currently are... It would leave the sidebar as-is (win), but, does add another layer of settings mgmt depth 🤷♂️ |
decided to go down this route... branch updated :) |
@setchy Looking good! How about we remove the "master" Logout? 🧐 |
Good callout. Done 🧹 🧼 |
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.
Code is good to my eyes. Really nice work in all the steps getting here, too. Great stuff!
Coverage decreased (-0.07%) to 96.904%
So demanding! Not an issue for me.
Coveralls really has no chill haha. It dropped when removing the master logout. 96+% coverage is still a mighty stat to be proud of |
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.
Looking good! Excited to see this live!
Add new
Accounts
view for managing authenticated accountsLogin with <x>
buttons have been moved from Settings view to the new Accounts viewLogout
(from all accounts) remains under SettingsFeedback appreciated on the UI - happy to iterate on it now, or in future.
![Screenshot 2024-06-03 at 9 04 13 PM](https://private-user-images.githubusercontent.com/386277/336269201-a2911b17-3aa8-42a4-afb1-288b1f56250f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjA1OTg1MTcsIm5iZiI6MTcyMDU5ODIxNywicGF0aCI6Ii8zODYyNzcvMzM2MjY5MjAxLWEyOTExYjE3LTNhYTgtNDJhNC1hZmIxLTI4OGIxZjU2MjUwZi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNzEwJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDcxMFQwNzU2NTdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02M2U3OTYxN2YyZWMzMjllNTJkYzlhMzk0NmIxYjM2NjUxOTQwYjllNGQyNTAyNGQxZmM1YWU0NDlhNzE2MmExJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.34RwcoLMbUxzGK4DvvuEJtBkmQPZP8mp1As_q-AkdHg)
Example of what it would look like when authenticated with multiple accounts
Note: I have still left the restriction on only 1 of each auth method as to relax this restriction require changes to our AccountNotification key and interaction methods (mark repo as read, native notifications, etc)