Skip to content

Conversation

nicolasgarnier
Copy link
Contributor

This depends on firebase/firebase-js-sdk#529

TODO: I guessed this will be in Firebase JS SDK 4.11.0 but need to double check that this is correct once the feature is released.

Change-Id: Id127ea77cbeddc525f357e624bb77f68b33d1ee0
Nicolas Garnier added 2 commits March 7, 2018 12:24
Change-Id: I111697f7a80950648e996e17585958b7d56398c0
Change-Id: Icfa44765d2abe8cba799f4e88df9fec002ec4c27
firebase.auth().signOut();
// [END signout]
// Re-enable the sign-in button.
document.getElementById('quickstart-sign-in').disabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be disabled at the end of this function since this executes synchronously?
I assume you only disable this during processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this was wrong. Also I'm not disabling the button when doing a sign-out. I moved disabled=true to the top and removed this disabled=false since that's handled in the onAuthStateChanged that this triggers...

document.getElementById('quickstart-sign-in').disabled = false;
} else {
var email = document.getElementById('email').value;
if (email.length < 4) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you do this specific check?

Copy link
Contributor Author

@nicolasgarnier nicolasgarnier Mar 8, 2018

Choose a reason for hiding this comment

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

It's just a crappy way of making sure we're passing some email value. (emails have to be at least 5 character long :D e.g. [email protected]) just tested and it looks likefirebase.auth().sendSignInLinkToEmail().catch checks nicely for formatted emails - I removed this crappy check. Which simplifies the code :) My only regrets is that when passing a blank or null email we could have came up with a slightly better message than "email badly formatted". Maybe something like "An email is mandatory, please provide one" would have been slightly better in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the problem is the same error is used for different contexts. We'll consider updating the error message.

alert('Error: ' + errorMessage);
console.log(error);
// Re-enable the sign-in button.
document.getElementById('quickstart-sign-in').disabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to re-enable this on successful sign-in too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the successful sign-in will trigger an onAuthStateChanged() which re-enables the button.

Change-Id: Icb6946a98d03fa12e214a0016959beff01a13545
Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

One nit and some open question.

});
// [END authwithemail]
}
document.getElementById('quickstart-sign-in').disabled = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't this anymore. it is called in the beginning.

firebase.auth().sendSignInLinkToEmail(email, actionCodeSettings).then(function() {
// Save the email locally so you don’t need to ask the user for it again if they open
// the link on the same device.
window.localStorage.setItem('emailForSignIn', email);
Copy link
Contributor

Choose a reason for hiding this comment

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

One advantage of using a cookie is that it will expire naturally after some point. localStorage will save this indefinitely if the action is not completed. The downside is that cookies are sent to server. Should we at least mention this?

Change-Id: If78c623d8bac70eb9ab82a0073c3554e93894b91
@bojeil-google bojeil-google merged commit 65991b8 into master Mar 20, 2018
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