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

Move global "Sign out" out of the current account row #4941

Merged
merged 8 commits into from
Aug 15, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 15, 2024

Implements #4775

Our "Sign out" button applies to all accounts but pretends to apply just to the current one. This is deeply confusing. There are good reasons for it to apply to all accounts (avoids tricky "logged into some but not all" cases), but we need to visually communicate what the button is doing, and to discourage its use in cases where it's not needed (account switch).

We move the "Sign out" button below the account list. Individual accounts now always have the "..." with "Remove account" instead. (You should be able to remove the current account when you need to, which you couldn't before.)

One account:

Screenshot 2024-08-15 at 02 27 15

Multiple accounts:

Screenshot 2024-08-15 at 03 37 15

The new "Sign out multiple accounts" at the bottom is more explicit that it clears everything.

The problem with the old "Sign out" buttons was that they're very tempting to press when you want to switch accounts (you want to "sign out", right?) But that's actually counterproductive (it logs you out of everything). By removing the prominent Sign out call to action, we reduce the likelihood of making that mistake. If you're planning to give the device to somebody you don't trust, you can still sign out — but not piecemeal.

Alternatively, you can remove the account.

Screenshot 2024-08-15 at 03 38 13

That would remove its tokens and its handle.

Initially I considered making it possible to sign out of some accounts but not others, but this is too confusing. It would still be possible to make a very bad mistake (signing out from one account while thinking you're signing out of all of them). And the mistake wouldn't even be obviously visible unless we add signed-in/out indication for each. A "remove" action is simpler.

Since it's now a bit more commonly displayed and it's a more destructive action in case you don't remember the handle, I've added a confirm prompt.

Screenshot 2024-08-15 at 03 42 41

The lack of a separate "Sign out" option here feels like a regression, but if we left it, we'd be silently changing the behavior of a very important button. Also, it would still be ambiguous.

To sum up, we now consistently show:

  • "Remove account" for each account (whether you have one or many)
    • I've added a prompt there to avoid accidental removals. Since remembering the handle can be tricky.
  • "Sign out of all accounts" (shortened to "Sign out" when you have one) at the bottom that does the old thing.

Test Plan

Go through the flows. Verify they make sense with one or more accounts.

In the case of having just one account, it's basically the same but the button moved down and you're able to remove it now.

Copy link

render bot commented Aug 15, 2024

Copy link

github-actions bot commented Aug 15, 2024

Old size New size Diff
7.09 MB 7.09 MB 902 B (0.01%)

@gaearon gaearon changed the title RFC: Make "Sign out" log out current account, add "Sign out of all accounts" Remove per-account "Sign out" (but leave "Remove account"), add "Sign out of all accounts" Aug 15, 2024
@gaearon gaearon changed the title Remove per-account "Sign out" (but leave "Remove account"), add "Sign out of all accounts" Move global "Sign out" out of the current account row Aug 15, 2024
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 15, 2024

Hm I guess this lost the indication which one you're signed into.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 15, 2024

Added "Other accounts" grouping to make it very clear which one you're signed in.

Alternatively we could do the checkmark. I don't love the one we're using though, and we'd have to place it next to the dropdown — don't want to remove the ability to remove the last remaining account.

We could also have some text label on the right liked "Signed in".

Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

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

Checked for both single and multiple accounts, looks good

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Added a small spacing tweak. LGTM!

@gaearon gaearon merged commit b6e515c into main Aug 15, 2024
6 checks passed
estrattonbailey added a commit that referenced this pull request Aug 21, 2024
* origin/main: (50 commits)
  Add `list hidden` screen (#4958)
  Expose more methods, support disabled items (#4954)
  Expose more props from button (#4953)
  Fix orphaned feed slices, handle blocks (#4944)
  Tweak `expo-modules-core` hack patch (#4955)
  [Experiment] Always show bottom bar (#4946)
  Revert "[Video] Download videos" (#4945)
  Move global "Sign out" out of the current account row (#4941)
  Hack patch for testing OTA update crash behavior (#4942)
  [Video] Download videos (#4886)
  swap control files (#4936)
  [Embed] Starter pack embed embed (#4935)
  [Video] set audio category to ambient every time a new player is made (#4934)
  Add `/live/` to supported YouTube embed URLs (#4932)
  [Video] Try/catch video play/pause (#4930)
  Don't kick to login screen on network error (#4911)
  Remove .withProxy() calls (#4929)
  [Video] Audio duck off main thread (#4926)
  subclass agent to add setPersistSessionHandler (#4928)
  [Video] Fix crash when switching tabs (#4925)
  ...
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