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 page and management improvements #1354

Merged
merged 11 commits into from
May 16, 2024

Conversation

CTalvio
Copy link
Collaborator

@CTalvio CTalvio commented May 6, 2024

Pull Request Description

  • Remove logout button from account page
  • Move the manage accounts button to the left (where log out used to be)
  • Enable logging out from the last account from the manage accounts modal
  • Do not display "you are browsing anonymously" placeholder when account is actually loading
  • Do not display account page before it has fully loaded (before, an empty view was momentarily displayed)
  • Change instance links in login dialogues to outline buttons
  • Add "log out" to general account actions on account setting page
  • Add "manage accounts" dialogue to account setting page (this also replaces the "you are browsing anonymously" placeholder when an anonymous account is used or is the only account available)

Issue Being Fixed

Various inconsistencies on the account page, alleviates confusion about where to find what and what the state of the account page is. Makes common actions available where one would think to look, especially when looking for account management via the settings tab. (before, you would never have found a way to log out from here, or add/remove accounts)

Screenshots / Recordings

image

image

Here's what switching accounts looked like before:
Screen_recording_20240506_224508.webm

After:
Screen_recording_20240506_224542.webm

Checklist

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

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

Just wanted to say, thank you so much for doing this! A lot of these things have been bugging me as I'm sure they've been bugging you (like the browsing anonymously thing when switching accounts).

From a UI/UX perspective, these are my only comments.

  • When using the in-app browser and navigating to one of the buttons (e.g., "Open Instance") the in-app browser is shown in the modal and is not scrollable. Most likely you didn't introduce this problem, so we can fix it later.
  • This is super nit-picky, but do you want to match the horizontal rule in the account settings page with the ones on the Debug and Appearance page (or vice versa) just for consistency?
  • If the current account is going to be switchable via "Manage Accounts" on the account settings page, should we make it not switchable by tapping the current account (if it feels redundant to you)? If so, that could revert from a UserSelector to a UserIndicator.
  • If I remove all accounts and then navigate to Settings > Account, I get an infinite spinner. Previously it would display the "you are browsing anonymously" screen, from which you could log in without leaving that page.
qemu-system-x86_64_3N5JP2lr0P.mp4

Otherwise everything else LGTM!

@CTalvio CTalvio added the on-hold Issue that requires more information or reproducible steps label May 6, 2024
@CTalvio
Copy link
Collaborator Author

CTalvio commented May 6, 2024

If I remove all accounts and then navigate to Settings > Account, I get an infinite spinner.

Good catch. And agreed on using a user indicator, the switchers are a bit redundant but I didn't bother figuring out something better, and what a timesave that was, as you now reveal the perfect widget exists already :D

This is super nit-picky, but do you want to match the horizontal rule in the account settings page with the ones on the Debug and Appearance page (or vice versa) just for consistency?

Yes. This would absolutely have bothered me the second I noticed.

@CTalvio CTalvio removed the on-hold Issue that requires more information or reproducible steps label May 6, 2024
@micahmo
Copy link
Member

micahmo commented May 7, 2024

Hi @CTalvio, sorry for being annoying! Your latest commit does fix the account settings page from settings! But now there seems to be an issue with the profile page.

  1. If I log out of the last account, it leaves my account info there.
  2. If I refresh, it gets stuck spinning forever.
qemu-system-x86_64_oXmAq2h42j.mp4

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 7, 2024

That seems to do the trick.

I'm not sure what the returns in those listeners were there to do?

@micahmo
Copy link
Member

micahmo commented May 7, 2024

Oh, we can't take those out! Those returns are what supports the temporary account feature (where you can post or comment as a different user). It allows us to switch the account without reloading the whole app. When reload is false, we shouldn't rebuild parts of the app like the user page or the feed. Sorry if that makes your feature more complicated (although I'm not sure why reload is false in your case).

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 7, 2024

Yeah I figured they were there for a reason... Hence my commenting on it.

The account page is leaving the old account page up because it doesn't update on changes elsewhere.

@micahmo
Copy link
Member

micahmo commented May 7, 2024

Would it help to do just the UI changes in this PR (moving the manage button the left, removing the log out button, allowing log out of last account, allow management of account from account settings page) and worry about the bloc/reloading issues separately? I know @hjiangsu has talked about combining AuthBloc and AccountBloc which I think could help us here too!

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 7, 2024

I'll poke at it some more, but if nothing else I'll obviously reverse removing those returns.

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 7, 2024

It's behaving weird in general. Switching the account while in account settings doesn't stick, but doing so from the account page, does. Removing the returns doesn't fix that.

@micahmo
Copy link
Member

micahmo commented May 9, 2024

This is super nit-picky, but do you want to match the horizontal rule in the account settings page with the ones on the Debug and Appearance page (or vice versa) just for consistency?

Just a note, I'm refactoring this divider widget (since we're using it a bunch now) in #1359, so whichever one of us goes first can pick up the changes from the other. 😊

@hjiangsu
Copy link
Member

I was playing around with this a bit, and noticed that if you manage accounts from the account settings, it changes your entire profile (until you refresh again?). Previously, switching users in the account settings would only temporarily switch users.

Here's what I mean:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-05-11.at.09.18.20.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-05-11.at.09.20.39.mp4

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 12, 2024

It's behaving weird in general. Switching the account while in account settings doesn't stick, but doing so from the account page, does. Removing the returns doesn't fix that.

It's supposed to actually change the account, unlike working the same way as posting/commenting as a different user like it did before. But it doesn't stick for some reason, as you point out.

This is because you can now remove/add accounts from account settings.

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 12, 2024

Mystery solved, I didn't remove the code that restores previous account upon popping out of the account settings.

As for the profile sticking around when going from logged-in to accounts removed/anonymous, due to the reload not happening for some reason, adding a logged-in check to the reload returns works around that oddity.

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 - just one nitpick

@hjiangsu hjiangsu merged commit ac6da24 into thunder-app:develop May 16, 2024
1 check passed
@CTalvio CTalvio deleted the account_changes branch May 16, 2024 16:17
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