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

Deprecate login option userEmail in favor of loginHint, also use with… #5313

Closed
wants to merge 2 commits into from

Conversation

@bradvogel
Copy link
Contributor

@bradvogel bradvogel commented Sep 29, 2015

… Google signin.

Fixes #2422

@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Sep 30, 2015

Tests failed but the look flaky.

@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Oct 1, 2015

@glasser @stubailo quick look over?

@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Oct 4, 2015

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 4, 2015

@bradvogel curious why you decided to change the option name - is it to be the same as a the Google API option name?

@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Oct 5, 2015

@stubailo it was a suggestion from @estark37 here: #2790 (comment)

options.loginHint = options.userEmail;
}

if (options && options.userEmail) {

This comment has been minimized.

@zimme

zimme Oct 5, 2015
Contributor

Shouldn't this be used only if loginHint isn't specified as on line 35?

options.loginHint = options.userEmail;
}

if (options && options.userEmail) {
loginUrl += '&user_email=' + encodeURIComponent(options.userEmail);

This comment has been minimized.

@zimme

zimme Oct 5, 2015
Contributor

or maybe you intended on using options.loginHint here?

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 5, 2015

@zimme thanks for jumping in with some comments!

@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Oct 5, 2015

Thanks guys. I meant loginHint instead userEmail - careless mistake. Thanks for the catch.

@zimme
Copy link
Contributor

@zimme zimme commented Oct 5, 2015

No problem, usually try and help out here and there whenever I feel like I have a minute over when looking around =)

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 5, 2015

Merged! 24718de

Thanks for the contribution, @bradvogel!

@stubailo stubailo closed this Oct 5, 2015
@zimme
Copy link
Contributor

@zimme zimme commented Oct 5, 2015

Also maybe document that loginHint can be some sort of sub string of the google account id when used with the google accounts system. Saw something about that in the docs @bradvogel linked to.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 5, 2015

Good point. @bradvogel could you add something about that in a new PR please?

@bradvogel bradvogel deleted the mixmaxhq:add-loginHint-oauth-param branch Oct 5, 2015
@bradvogel bradvogel mentioned this pull request Oct 5, 2015
@bradvogel
Copy link
Contributor Author

@bradvogel bradvogel commented Oct 5, 2015

stubailo pushed a commit that referenced this pull request Oct 5, 2015
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants