thunder-app / thunder

Thunder - An open-source cross-platform Lemmy client for iOS and Android built with Flutter
https://thunderapp.dev
GNU Affero General Public License v3.0
732 stars 62 forks source link

Temporarily change account in settings page #1171

Closed micahmo closed 2 months ago

micahmo commented 2 months ago

Pull Request Description

Using the same technique as #1159, this PR allows for configuring the account settings as any of the available users. I also condensed some of the "account restoration" functionality so that it stays within whatever page may invoke a temporary account.

Note: I have introduced some usage of PopScope. I do remember that WillPopScope was a no-no because it broke full page swipe back on iOS; however I think we got around that by using SwipableaPageRoute. Also this is the newer PopScope. All this to say, can you verify that full page swipe back still works on iOS from the account settings page?

Review without whitespace.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

https://github.com/thunder-app/thunder/assets/7417301/771e4ffd-53fb-42ee-b44c-47360b3339e5

Checklist

hjiangsu commented 2 months ago

All this to say, can you verify that full page swipe back still works on iOS from the account settings page?

Just tested it out, and it seems to be working as expected!

micahmo commented 2 months ago

We do have a lot of places which rely on checking account bloc state, so once this is merged, just be on the look out for anything that might seem off (e.g., subscriptions, favourites, etc)!

Will do! I mainly wanted to prevent reloading the main feed and the user page. If other places reload for each switch and then reload back to the original, it's probably not the end of the world, but we can add additional reload checks anywhere, now that we have the flag in the state.

hjiangsu commented 2 months ago

I do wonder if perhaps, a more elegant way to handle this is to create a new AccountBloc when we're navigating to a screen that allows us to switch accounts (e.g., create post page, create comment page, account settings page, etc).

This way, the "global" account state is not touched, and we push a new AccountBloc for the "local" state. This is how the feed is currently being handled ("global" FeedBloc for the general feeds/subscribed communities) and "local" FeedBloc for any communities that are pushed to a new page). I hope that makes sense!

micahmo commented 2 months ago

That could work, but it might not be enough. The first time I attempted this feature, I simply added a "temporary" account field, but there were so many different ways that the "current" account can be accessed, it was hard to handle all the cases. The easier approach was to switch everything and handle the cases that shouldn't respond to the switch.

For example, we would probably need to push a new AuthBloc as well. And there's also fetchActiveProfileAccount which goes directly to the SharedPreferences.

Anyway, something to think about.

hjiangsu commented 2 months ago

Ahh yeah, you have some good points! It would likely take more work than what I mentioned to get everything working properly 😅

We'll stick to this for now and possibly revisit it in the future!

machinaeZER0 commented 2 months ago

Heyyy! I just saw this in the pre release notes, this is cool! I wonder if the ability to change the account you're viewing by tapping the user area at the top of Account Settings should be made more obviously clickable? A three dot/hamburger menu might be one option here.

I could file a feature request if that felt like a good tweak - let me know!

micahmo commented 2 months ago

Hey, I think that's a good idea. This UI originally started in the create post page, where the account and community selectors look/act the same. The difference is that the community may not be populated initially (if you create a new post from the main feed), so it's obvious that you have to tap it, where the account selector is always populated by default, so it would definitely be a discoverability issue (unless you happened to assume that it had the same behavior as the community selector).

All that to say, yes, feel free to create an enhancement issue for this.

micahmo commented 2 months ago

@machinaeZER0 Here are some quick mockups.

image image

Here's another idea.

image image
machinaeZER0 commented 2 months ago

Aw, thanks for mocking these up! Upon reflection, I guess maybe the three dot icon implies a pop-up style menu, as opposed to a slide out panel...

On our profile page we're actually using that icon in the top right as an account switcher (the one that of the two user icons overlapped) - maybe that would be the icon to use, for consistency? The right-facing arrow also works though, I think. Sorry for not thinking of that initially!

micahmo commented 2 months ago

Thanks for the feedback! I don't think the three dots is necessarily bad. A bottom sheet is very similar to a dialog in that they're both modals.

we're actually using that icon in the top right as an account switcher (the one that of the two user icons overlapped) - maybe that would be the icon to use

That's also possibility! My main concerns there would be (a) we don't actually want to imply that you're switching accounts for the whole app, so maybe "consistency" isn't what we want 😆 and (b) it might look funny for the community selector to have a different icon? It might actually be more important to maintain consistency between those two operations, since they both only apply to the current screen.

image
machinaeZER0 commented 2 months ago

That makes a ton of sense to me! Defer to you all on final icon choice :) either way I think this is great!

hjiangsu commented 2 months ago

Just going to chime in on this discussion - I think I prefer the chevron icon more than the 3-dot icon. It feels more intuitive for me (but that might just be me 😅)

micahmo commented 2 months ago

Whew, that's what I ended up doing! 😊

machinaeZER0 commented 2 months ago

TIL it's called a Chevron icon!