Pullrequest equipo Bichigram#30
Conversation
Added basic structure of the site (SPA) + component files.
You can login with an existing account, if the user exists it redirects you to the feed, if it doesn't it sends an alert message (test account -- email: test@testing.com password: 123456).
HectorBlisS
left a comment
HectorBlisS
left a comment
There was a problem hiding this comment.
Excelente, se nota que revisaron a fondo la documentación de Firebase. Siento que es poco visible el trabajo de todas, es necesario arreglar el github, pues los commits aparecer como anonimos o mixtos y es importante diferenciar el trabajo de cada una. Felicidades por usar Sass y el trabajo con JavaScript se nota mucho mejor a los proyectos anteriores. Muy bien.
| // // 🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥 | ||
| // // The Firebase SDK is initialized and available here! | ||
| // | ||
| // firebase.auth().onAuthStateChanged(user => { }); | ||
| // firebase.database().ref('/path/to/ref').on('value', snapshot => { }); | ||
| // firebase.messaging().requestPermission().then(() => { }); | ||
| // firebase.storage().ref('/path/to/ref').getDownloadURL().then(() => { }); | ||
| // | ||
| // // 🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥 |
There was a problem hiding this comment.
Hay que evitar que Firebase sobreescriba tus archivos html
|
|
||
|
|
||
| const init = () => { | ||
| changeView(window.location.hash) |
There was a problem hiding this comment.
Si se usa un hash router es importante crear un archivo para colocar las rutas
|
|
||
| const initFirebase = () => { | ||
| // Your web app's Firebase configuration | ||
| var firebaseConfig = { |
| divElement.innerHTML = viewLogin | ||
|
|
||
| //nodes (to get the DOM elements inside the form and initialize the login function) | ||
| const emailText = divElement.querySelector('#emailText') |
There was a problem hiding this comment.
Muy bien la semántica de los nodos
|
|
||
| //sign in with firebase functions | ||
|
|
||
| const promise = auth.signInWithEmailAndPassword(email, pass) |
There was a problem hiding this comment.
Excelente el manejo de la promesa
| @@ -1,36 +1,80 @@ | |||
| import { changeView } from '../view-controler/router.js' | |||
|
|
|||
| // eslint-disable-next-line import/no-cycle | |||
There was a problem hiding this comment.
Muy bien la separación de funciones en archivos respectivos
| location: loc, | ||
| date: firebase.firestore.Timestamp.fromDate(new Date()), | ||
| counter: 0, | ||
| }).then(console.log('ok C:')); |
There was a problem hiding this comment.
Quitamos los console.log que no utilizamos al final
| postimg: url, | ||
| description: descr, | ||
| location: loc, | ||
| date: firebase.firestore.Timestamp.fromDate(new Date()), |
There was a problem hiding this comment.
Muy bien el uso del formato de Firebase
|
|
||
| // upload the image to storage | ||
| storageRef.put(image) | ||
| .then(snap => snap.ref.getDownloadURL()) |
| const p = document.querySelector('#posts'); | ||
| p.innerHTML = ''; | ||
| snap.forEach((doc) => { | ||
| const div = `<div> | ||
| <p>${doc.data().date}</p> |
There was a problem hiding this comment.
Evitemos mezclar el DOM con la DB, esto debería estar en una función que devuelve un array con los docs y en otra se dibujan
| <script src="main.js" type="module"> </script> | ||
|
|
||
| <!-- The core Firebase JS SDK is always required and must be listed first --> | ||
| <script src="https://www.gstatic.com/firebasejs/7.14.0/firebase-app.js"></script> |
There was a problem hiding this comment.
al final si se borra el "-app" quedando solo https://www.gstatic.com/firebasejs/7.14.0/firebase.js
se ahorrarían algunas lineas de código
reloadercf
left a comment
reloadercf
left a comment
There was a problem hiding this comment.
Las recomendaciones que realizo @HectorBlisS me parecen adecuados.
|
|
||
| <!-- TODO: Add SDKs for Firebase products that you want to use | ||
| https://firebase.google.com/docs/web/setup#available-libraries --> | ||
| <script src="https://www.gstatic.com/firebasejs/7.14.0/firebase-analytics.js"></script> |
There was a problem hiding this comment.
Firebase analytics no se utiliza en este proyecto ;)
4 participants
Me parece que ya está todo, me gustaría seguir haciendo mas pruebas de testing por namas quedaron 3.