Gh login km #48
Gh login km #48
Conversation
|
Thanks for the quick PR @kammitama5 :) You can run your application in Cloud9 by clicking the "Run" or the "Preview" button from the top menu bar and it'll open a web page with your app. You'll just have to navigate to the See the Cloud9 official docs page here on how to run your app: https://docs.c9.io/docs/run-an-application and let me know if that helps! |
|
Thanks! |
LearningNerd
left a comment
LearningNerd
left a comment
There was a problem hiding this comment.
Thanks @kammitama5, great start here!! I posted a bunch of inline comments addressing specific lines of code with suggestions. Keep at it! Let me know if you have questions. :)
| console.log("User successfully logged in to Firebase!"); | ||
|
|
||
| //Here: update the paragraph with ID of "userinfo to display Github's Username and Github profile photo " | ||
| //Otherwise, if no user currently logged in: |
There was a problem hiding this comment.
This comment on line 75 should probably be moved down to around the else statement on line 83, or just remove the comment altogether to avoid any confusion
| console.log("User successfully logged OUT from Firebase!"); | ||
|
|
||
| // Here: Update the paragraph with ID of "userinfo" to display the message "Not currently logged in." | ||
| document.getElementById("userinfo").textContent = "Not currently logged in"; |
There was a problem hiding this comment.
Since you already created a variable for this element on line 21, you may as well use it here instead of generating the JavaScript object again with document.getElementById() a second time -- keep your code DRY (https://en.wikipedia.org/wiki/Don%27t_repeat_yourself)! :)
| //Here: update the paragraph with ID of "userinfo to display Github's Username and Github profile photo " | ||
| //Otherwise, if no user currently logged in: | ||
| var username = document.getElementById("username"); | ||
| var profilephoto = document.getElementById("profilephoto"); |
There was a problem hiding this comment.
Line 76 and 77 are trying to access an element in your HTML file with the IDs "username" and "profilephoto", but they don't exist! You want to be accessing the Firebase user object instead here.
|
|
||
| // Display Username and Photo | ||
| var show = document.getElementById("userinfo").innerHTML; | ||
| var show1 = document.getElementById("profilephoto").innerHTML; |
There was a problem hiding this comment.
What are the show and show variables for here? I'm not sure I follow.... But remember, here you want to change the value of the innerHTML property belonging to that one new paragraph you created earlier for this challenge. By changing its innerHTML property to a new value, you can display not just text inside that paragraph but also more HTML tags, like an image!
|
|
||
| // Event Listener for Login | ||
| // Use Firebase with Github Auth to log in the user | ||
| firebase.auth().signInWithRedirect(provider).catch(function(error){ |
There was a problem hiding this comment.
I don't see any event listener here though! When should the user be redirected to log in? What event are you listening for from the user, and where exactly is that event going to happen on the HTML page? Take a look at previous examples of JavaScript event listeners and use one as a reference.
|
|
||
| // Event Listener for Logout | ||
| // Change your logout button's event listener | ||
| firebase.auth().signOut().catch(function(error){ |
There was a problem hiding this comment.
Same issue as on line 54 -- I don't see an event listener here! See my comment on line 54 for this section too. Same questions apply here: when should the user be logged out?
| var dbUsername = dbRef.child("myname"); | ||
|
|
||
| // button function and event listener | ||
| document.addEventListener("click", myButton); |
There was a problem hiding this comment.
I'm a bit confused by this event listener -- did you get to test it? Seems like it wouldn't work right, or if this is leftover code from a temporary experiment, remember to delete it when you make your next edits. See my comments on line 54 and 62 below regarding the event listeners.
|
|
||
| // Whenever "myname" value in our database is updated, show the data inside usernameBox! | ||
| dbUsername.on("value", function(dataSnapshot) { | ||
| usernameBox.textContent = dataSnapshot.val(); |
There was a problem hiding this comment.
Sorry I must have missed this in your last pull request -- you actually should have two Firebase event listeners -- one for the greeting, and one for the username. So you should remove line 48, then copy lines 40 to 50, paste a duplicate, and change that copy to listen for changes to your other Firebase reference (dbUsername) and modify your other paragraph (usernameBox) accordingly.
I'm not able to do the browser tests for this(I'm not sure how to do it in Cloud9, so have to poke around...poke...not poké), so it looks like I may actually need help for this, but here is my base. I'm also going to try to clone this locally today (or tomorrow) and see if I can get it to work and troubleshoot there. That might work, but would prefer to get it up and running in cloud9 so I don't have instances of my project all over...