andymatuschak / orbit

Experimental spaced repetition platform for exploring ideas in memory augmentation and programmable attention
https://withorbit.com
Other
1.71k stars 54 forks source link

Add 'continue with user' option when credentials are already present #217

Closed kirkbyo closed 3 years ago

kirkbyo commented 3 years ago

Closes https://github.com/andymatuschak/orbit/issues/179

What When the credentials are already present on the iframe's domain, a "Continue with " option is shown to confirm with the user that they are already authenticated.

Demo of Before and After behaviour | Before | After | | --- | --- | | ![old_flow](https://user-images.githubusercontent.com/8084674/120401028-ee8d2100-c2f3-11eb-8daa-535e0e03bfed.gif) | ![new_flow](https://user-images.githubusercontent.com/8084674/120401038-f6e55c00-c2f3-11eb-8b3b-c6e7f82686a0.gif) |

How If the userRecord is received before the user has entered their authentication credentials, we'll "double check" that they want to use the account we found. I added a new "ContinueWithUsercomponent to handle this behaviour to avoid bloatingSigninForm` further with conditionals.

Notes I will admit this new flow feels a bit "wonky". It feels weird to ask the user to "continue" without presenting them another option. I attempted to add a button that allows the user to "Use a different account", but to do this flow properly would involve reworking how the overrideEmailAddress is used right now. If we did want this behaviour, I'd be happy to take that on in a separate PR!

andymatuschak commented 3 years ago

Nice work, @kirkbyo! I'll comment at a high level before digging into the code changes.

At first, I thought: oh, boy, I guess we really have to make the sign in flow much more fleshed out to make this reasonable. To your point, it's weird not to have a second option!

But then I realized: hey, wait. In this flow, the user has already entered their email address in the prior screen. This is quite different from other OAuth flows, where they really need to clarify that you intend to sign in using the saved credential. In our case, if the saved credential's email address matches the one you entered, that's what you mean! This lets us simplify the UI language, and also means we don't need an alternative option.

So if the user's signed in, I think the secondary text can say: "Welcome back, EMAIL. We signed you in using a saved password." and the button can say "Continue".

Now, the other thing this makes me realize is that if the stored credential doesn't match the email address they entered, we should discard it, sign out, and show the normal login interface. After all, they asked to sign in with a different address! A fancier flow might say "hey, wait, did you want to sign in with OTHER_EMAIL instead?" (people often forget which email address they use for things), but I think we can skip that for now.

andymatuschak commented 3 years ago

Oh, and probably we should focus() the button on mount, so that people can just hit return / space / whatever to continue.

kirkbyo commented 3 years ago

Makes sense! I have updated the auth flow with the changes you mentioned:

Screen Shot 2021-06-02 at 12 11 22 PM

Demo ### Flow for when the `userRecord` and the `overrideEmailAddress` match ![new_flowV2](https://user-images.githubusercontent.com/8084674/120540315-64999280-c39d-11eb-8af2-96ed3d603143.gif) ### Flow for when the `userRecord` and the `overrideEmailAddress` __do not__ match ![different_email](https://user-images.githubusercontent.com/8084674/120540977-2355b280-c39e-11eb-95c6-02b9ced025d2.gif)
kirkbyo commented 3 years ago

Done! I will merge once the tests go green.

This patch reminds me that we’re probably just creating pain for ourselves by separating ui into a separate module like this. Perhaps at some point we’ll want to just move it into app. Or maybe just simple components like Button should be in a separate ui module? Or maybe a separate style module just for static bits like color / type hierarchy? Not sure.

My two cents: I personally like the idea of separating the UI from the app since it forces you to not encapsulate API logic within the UI and allows for testable components like SigninForm. However, I do think the folder could be separated into design system components (i.e. Button) vs. views (SigninForm or ReviewArea). Also, I worry that moving the design system bits into a separate package would become tedious at this stage since we'd now have two storybook instances.

andymatuschak commented 3 years ago

Those are all good points! 👍