Skip to content
This repository was archived by the owner on Jul 11, 2026. It is now read-only.

11_Andrea Toledo, Laura Bailón, Yaki Romero#21

Open
Yaki-Zeltzin wants to merge 33 commits into
Laboratoria:masterfrom
Yaki-Zeltzin:master
Open

11_Andrea Toledo, Laura Bailón, Yaki Romero#21
Yaki-Zeltzin wants to merge 33 commits into
Laboratoria:masterfrom
Yaki-Zeltzin:master

Conversation

@Yaki-Zeltzin

Copy link
Copy Markdown

code review

Comment thread src/style.css
@@ -256,9 +283,9 @@ html {
#search {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Es más recomendable utilizar clases para dar estilos.

Comment thread src/comments.js
"especialidad": snapshot.val().profile.especialidad

}
const updates2 = {};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Utilicen nombres de variables más descriptivos, updates2 no me dice nada.

Comment thread src/comments.js
}
function setLikePost() {
let uid = firebase.auth().currentUser.uid;
let name = firebase.auth().currentUser.displayName ? firebase.auth().currentUser.displayName: firebase.auth().currentUser.email;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Qué bueno que utilicen ternarios!

Comment thread src/data.js
// Email sent- email enviado
console.log("Enviando correo..");
}).catch(function(error) {
// An error happened.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Eliminar los comentarios innecesarios

Comment thread src/main.js Outdated


/*// BOTON LOGIN CON GOOGLE
//BOTON LOGIN CON GOOGLE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lo mismo de los comentarios

Comment thread src/post.js Outdated
"especialidad": snapshot.val().profile.especialidad

}
const updates2 = {};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

el nombres de variables es importante

Comment thread src/auth.js
@@ -1,7 +1,12 @@
// LOGIN CON EMAIL Y PWD

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

eliminar este comentario

Comment thread src/auth.js
return firebase.auth()
.signInWithEmailAndPassword(email, password)
.catch(function(error) {
// Handle Errors here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Estos comentarios deben eliminarse

Comment thread src/auth.js Outdated
.catch(function(error) {
// Handle Errors here.
const errorCode = error.code;
const errorMessage = error.message;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hay que eliminar todos los comentarios innecesarios

Comment thread src/auth.test.js

auth.emailLogin(email, password).then(valor => {
expect(valor).toBe(true)
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Que pro se ven estas pruebas! muy bien!

Comment thread src/auth.js Outdated
alert("Contraseña incorrecta")
}
console.log(`${errorCode} ${errorMessage}`)
// ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hay que poner atención en lo que subimos cuando hacemos push, los console se deben eliminar.

Comment thread src/style.css Outdated
<<<<<<< HEAD

=======
>>>>>>> 4b557f0fee0b6b5db36d549f21a7071b628e9a4a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

esto es de un conflicto, lograron resolverlo?

Comment thread src/auth.test.js Outdated
});
});

*/ No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

De nuevo aquí, hay que poner mucha atención en el código que no estamos utilizando.

Comment thread src/index.html
<script src="ya.js"></script>
<script src="p.js"></script>
<script src="post.js"></script>
<script src="public.js"></script>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Por qué hay tantos archivos linkeados de js? no se hizo por módulos?

Comment thread src/main.js
<span id="google-login" class="login"><img src="./images/g.png" class="icon" alt="googleLogo"><p>Iniciar Sesión con Google</p></span>
<span id="facebook-login" class="login facebook"><img src="./images/f.png" class="icon" alt="facebookLogo"><p>Iniciar Sesión con Facebook</p></span>
</div>
`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hay que cuidar la indentación y el formato de sus archivos

Comment thread src/main.js
</div>
<div id="logins">

<p class="WelcomeFont">Bienvenidx</p>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hay que ser consistentes con el naming en nuestras clases y todos lo atributos y variables que utilicemos, si está utilizando guiones y lowercase entonces debió ser la clase: welcome-font

Comment thread src/post.js
<a id="edit-comment-${snapshot.val()[comment].postId}/comments/${comment}" class="edit-comment" >Editar</a>
<a id="delete-comment-${snapshot.val()[comment].postId}/comments/${comment}" class="remove-comment">Eliminar</a>
</div>
</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hay que cuidar la indentación y formato de sus archivos.

Comment thread src/post.js
commentsRef2.remove();
} else {
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Qué hace este return?

Comment thread src/post.js
}

//Function delete comments

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Estos comentarios son innecesarios.

Comment thread src/post.js
function cancel_editPost(){
let cancelId=this.id.slice(12)
document.getElementById("post-user"+cancelId).disabled = true;
document.getElementById("button-option"+cancelId).style.display= "none";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Aquí también podríamos utilizar template strings:
button-option ${cancelId}

Comment thread src/post.js
let editCommentUser=this.id.slice(13)
document.getElementById("comentUser"+editCommentUser).disabled=false;
document.getElementById("comentUser"+editCommentUser).focus();
document.getElementById("buttonConfirmComment"+editCommentUser).style.display= "block";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Igual aquí.

Comment thread src/post.js
function cancel_change_comment(){
let cancelIdComment = this.id.slice(13)
document.getElementById("comentUser"+cancelIdComment).disabled = true;
document.getElementById("buttonConfirmComment"+cancelIdComment).style.display= "none"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lo mismo acá

Comment thread src/style.css
height: 80px;
background-color: whitesmoke;
background-color: rgb(56, 10, 83);
/* background-color: rgb(33, 126, 156); */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

borrar comentarios innecesarios.

Comment thread src/auth.js
// An error happened.
return false
//return false
console.log(error)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todos estos comentarios deben eliminarse

Comment thread src/auth.test.js
*/ No newline at end of file
expect(falsesignInWithEmailAndPassword(email, password)).toBe('Email inválido');
expect(auth.emailLogin(password)).toBe('No cumple con los 6 caracteres');
});*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todo este archivo está comentado, si no se utiliza debe eliminarse.

Comment thread src/main.js
}

// FUNCION QUE CREA PAGINA INICIAL
function loginPage() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Eliminar los comentarios innecesarios.

@AdrianaHY AdrianaHY left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Chicas, me parece que lo que han logrado hasta ahora es impresionante!
Por otro lado, continuan con bastante problemas en cuanto a buenas prácticas que se deben mejorar, también hay unas partes que se pueden refactorizar o hacer de otra manera, por ahora lo que pude ver fue la concatenación que se puede sustituir con un template string.

Por lo demás muchas felicidades! continuen con su buen trabajo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants