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

Temporarily change account in settings page #1171

Merged

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Mar 6, 2024

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

qemu-system-x86_64_7mVGeFFOvv.mp4

Checklist

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

@hjiangsu
Copy link
Member

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!

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.

LGTM! 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)!

@hjiangsu hjiangsu merged commit 1102a31 into thunder-app:develop Mar 11, 2024
1 check passed
@micahmo micahmo deleted the feature/account-settings-switch branch March 11, 2024 15:58
@micahmo
Copy link
Member Author

micahmo commented Mar 11, 2024

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
Copy link
Member

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
Copy link
Member Author

micahmo commented Mar 11, 2024

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member Author

micahmo commented Mar 28, 2024

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
Copy link
Member Author

micahmo commented Mar 28, 2024

@machinaeZER0 Here are some quick mockups.

image image

Here's another idea.

image image

@machinaeZER0
Copy link
Collaborator

machinaeZER0 commented Mar 28, 2024

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
Copy link
Member Author

micahmo commented Mar 28, 2024

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
Copy link
Collaborator

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

@hjiangsu
Copy link
Member

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
Copy link
Member Author

micahmo commented Mar 28, 2024

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

@machinaeZER0
Copy link
Collaborator

TIL it's called a Chevron icon!

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.

3 participants