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

Account improvements and anonymous instances #677

Merged
merged 13 commits into from
Sep 21, 2023

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Aug 23, 2023

Pull Request Description

Buckle up, this is a big one!

This PR introduces a number of changes related to anonymous browsing and user management.

  1. Display the current user (or anonymous), including avatar, at the top of the sidebar.
    • Tapping will navigate to the profile page.
    • Pressing the three dots will show the account switcher.
  2. Anonymous instances are now supported.
    • Any number of anonymous instances may be added removed and switched to at any time.
  3. The account switcher flow has been improved.
    • The current account/instance can be logged out directly in the switcher.
    • Logging out of the current account/instance will switch to (a) the last anonymous instance, if no more accounts, or (b) the last account, if no more instances. (This improves the current behavior where you are "stuck" on the instance of the last account when signing out.)

I tried to be very thorough with my testing, but this is a case where we need a QA team 😆 If anyone wants to grab this branch and provide additional testing, that would be appreciated! I'm especially thinking about potential regressions in anonymous subscriptions (@vbh). But it's a pretty wide-ranging change that may have other regressions as well.

Note: There will be conflicts between this and #655, and I'd prefer that one to go first.

Review without whitespace!

Issue Being Fixed

Issue Number: #269, #491

Screenshots / Recordings

New user section in the drawer

accounts-1.mp4

Add anonymous instances

accounts-2.mp4

Log out will revert to another account/instance if available

accounts-3.mp4

Just showing that account/instance switching propagates everywhere properly

accounts-4.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@machinaeZER0
Copy link
Collaborator

LOVE this!! I'd mentioned in a feature request, but could the hamburger menu icon be replaced with the user avatar with this change? This would serve a dual benefit since you'd be able to see at a glance which account you're using, without even opening the sidebar (assuming the different accounts are using different avatars, at least)

@machinaeZER0
Copy link
Collaborator

Issue #607 was the one I referenced above

@micahmo
Copy link
Member Author

micahmo commented Aug 24, 2023

could the hamburger menu icon be replaced with the user avatar with this change

This is an interesting idea. My concern is that it might be slightly unusual for tapping on an avatar to open a drawer. For example in the Google apps, it opens a little modal which contains mostly account-related actions. The Google apps often also have a separate drawer. Or if you look at Sync, the profile icon is only shown when you disable drawer navigation (and pressing it brings up the account switcher modal), otherwise the hamburger icon is shown.

That said, all these concerns go away if it's an option, so I'm not opposed to the idea. 😉 Since this PR is already so huge, I'd probably want to do is separately.

@shortwavesurfer2009
Copy link

Actually, tusky uses the avatar as app drawer icon. Example
Screenshot_20230824-152823_Tusky_1
Screenshot_20230824-152941_Tusky

@micahmo
Copy link
Member Author

micahmo commented Aug 24, 2023

Ah, nice counter example!! I'm definitely not opposed to this but again would prefer to do things in stages if that's ok. 😊

@machinaeZER0
Copy link
Collaborator

Absolutely!

@hjiangsu
Copy link
Member

hjiangsu commented Sep 7, 2023

This does seem like a pretty big change, so I'll probably hold off on merging this in until after the next general release if thats all good!

@machinaeZER0
Copy link
Collaborator

Awww but it's so coooool 😄

@micahmo
Copy link
Member Author

micahmo commented Sep 7, 2023

This does seem like a pretty big change, so I'll probably hold off on merging this in until after the next general release if thats all good!

I'm good either way. On the one hand, we're "early" in the release cycle (only just published the first prerelease) so we have plenty of time to get this tested. On the other hand, I'm guessing we're going to try to push a new GA faster than usual due being absent for a little while.

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

Oh man, this is a big one - I mainly did a skim through rather than a thorough review on this one, but it looks pretty good overall!

I just had a few points of discussion in some places, but I'm okay with handling those changes in a different PR if needed. Anyways, can you confirm if this is all ready to go to be merged in?

lib/thunder/pages/thunder_page.dart Outdated Show resolved Hide resolved
lib/thunder/bloc/thunder_bloc.dart Show resolved Hide resolved
@micahmo
Copy link
Member Author

micahmo commented Sep 21, 2023

I mainly did a skim through rather than a thorough review on this one

Haha fair enough! I'm hoping if we get it in early, we can get plenty of feedback.

can you confirm if this is all ready to go to be merged in?

It should be all set! I recently retested after merging in the latest develop. Let me look at your comments though.

@micahmo
Copy link
Member Author

micahmo commented Sep 21, 2023

All set now!

@hjiangsu hjiangsu merged commit 155ac86 into thunder-app:develop Sep 21, 2023
1 check passed
@micahmo micahmo deleted the feature/anonymous-instances branch September 21, 2023 20:32
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.

4 participants