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.

Sign up for GitHub

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

Jump to bottom

Link multiple auth providers #618

Merged
merged 3 commits into from
Feb 28, 2020
Merged

Link multiple auth providers #618

merged 3 commits into from
Feb 28, 2020

Conversation

liyasthomas
Copy link
Member

@liyasthomas liyasthomas commented Feb 28, 2020 •
edited
Loading

This is the hacky solution- Open alternate signin popup on error catch.

  • Google
  • GitHub
  • Combine login flow in login.vue and settings.vue page.

@ghost
Copy link

ghost commented Feb 28, 2020 •
edited by ghost
Loading

DeepCode's analysis on #154c2b found:

  • ❌ 0 critical issues. ⚠️ 0 warnings and 1 minor issue. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.

☺️ If you want to provide feedback on our bot, here is how to contact us.

@liyasthomas liyasthomas changed the title ⚗️ Link multiple auth providers Link multiple auth providers Feb 28, 2020
@TravisBuddy
Copy link

Hey @liyasthomas,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 46036810-59c1-11ea-822e-0bf67ba4204b

@TravisBuddy
Copy link

Hey @liyasthomas,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: cd435c20-59c3-11ea-822e-0bf67ba4204b

@dihmeetree
Copy link
Contributor

How do I test this commit using the Netlify preview build link? @liyasthomas I press login with Google/Github and a box pops up, and then closes. Should I do it on localhost? Is Firebase Auth setup to allow logins from Localhost?

@liyasthomas
Copy link
Member Author

liyasthomas commented Feb 28, 2020 •
edited
Loading

only production and localhost:300 work with Firebase Auth

@dihmeetree
Copy link
Contributor

only production and localhost work with Firebase Auth

Gotcha! Will take a look :)

@dihmeetree dihmeetree self-requested a review February 28, 2020 01:52
@dihmeetree
Copy link
Contributor

dihmeetree commented Feb 28, 2020 •
edited
Loading

@liyasthomas not sure how your PR was working on your local 😄 But; I fixed a few scoping issues, and issues with toast notifications (within Firebase function callbacks). I also added a notification, for when you successfully login with Firebase!

This PR should fix any issues I saw, however; there could be others.

Note: I didn't look too far into how i18n works in this project juet yet, and if a key is missing in one of the lang JS files.. will it fallback to a key in the EN file? That's the only thing I'm wondering, but didn't look into. If there are isssues with users from other countries not seeing toast boxes.. that could be a root cause.

dihmeetree
dihmeetree approved these changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@dihmeetree dihmeetree left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added my review comment, and merge on the PR. LGTM @liyasthomas

@dihmeetree dihmeetree added bug fix This fixes a bug critical Better handle this first labels Feb 28, 2020
@TravisBuddy
Copy link

Hey @liyasthomas,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: a300c9c0-59d7-11ea-af10-7fd2d08fe4df

@dihmeetree dihmeetree merged commit 0694f46 into master Feb 28, 2020
@liyasthomas liyasthomas deleted the feature/auth branch March 4, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dihmeetree dihmeetree dihmeetree approved these changes

Assignees
No one assigned
Labels
bug fix This fixes a bug critical Better handle this first
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liyasthomas @TravisBuddy @dihmeetree