Skip to content

Conversation

@mythmon
Copy link
Contributor

@mythmon mythmon commented Sep 18, 2013

This helps the case where Persona takes a little while to do the assertion dance. It seems a little confusing for Persona to go away, and no more instructions be given to the user, but the button still says "Sign in with email". This fixes that by changing the text to something that implies something is happening. I chose this text because it is the same text that is shown on the Persona popup.

I'm not sure in what cases onCancel will be called, but it is mentioned in the docs, and I assume it will happen anytime a failure happens. The docs say onCancel is "A function that will be invoked if the user refuses to share an identity with the site."

r?

@rlr
Copy link
Contributor

rlr commented Sep 18, 2013

This is neat, but I don't think it solves all cases and, specifically, the case Kadir ran into.

The problem is when you are confirming your email manually, it opens a link that redirects to the page you were on SUMO. Then this page loads and the watch api kicks in and at some point realizes you are logged in and reloads the page. Since this is a brand new tab, nothing has been clicked on it. I'm not sure what we can do to detect this was from the email and we are likely waiting for browser id.

I am r+ on this fix. But there is more to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK! Why would they do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed 6723c49, which changes the case of that param.

This is actually even better now, because when the user closes the Persona window, it calls oncancel, which is kind of what I expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

yay!

@mythmon
Copy link
Contributor Author

mythmon commented Sep 18, 2013

Right this, is just a fix for when users click the button directly, not when the watch api kicks us. The two-sumo-tabs problem is something I haven't thought about, and it is a bit lame.

@mythmon mythmon merged commit 067a4fb into mozilla:master Sep 18, 2013
@mythmon
Copy link
Contributor Author

mythmon commented Sep 18, 2013

067a4fb

@mythmon mythmon deleted the persona-login-feedback branch September 18, 2013 20:36
@rlr
Copy link
Contributor

rlr commented Sep 19, 2013

i wonder if we can do some trick with localstorage to keep track of the state of logging in across tabs. or mostly just for new tabs that get opened.

@mythmon
Copy link
Contributor Author

mythmon commented Sep 19, 2013

I don't follow. What would this gain us? Since we are switching to the watch api (right?) Persona will notify us when the user is logged in.

@rlr
Copy link
Contributor

rlr commented Sep 19, 2013

So the problem is that the watch api cant take it's good time to kick in once a fresh new tab is opened on sumo. Which is what happens when you click the confirm email link in the persona activation email.

@mythmon
Copy link
Contributor Author

mythmon commented Sep 19, 2013

Oh, that make sense. I just did that sign in process and watched both windows at once. The first window doesn't know you signed in any sooner than the other, but we could do something like localstorage.persona_sign_started = now and then if that is recent, show "completing sign in" instead of "Sign in with email."

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.

2 participants