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

Code review#17

Open
lauraac wants to merge 26 commits into
Laboratoria:masterfrom
lauraac:master
Open

Code review#17
lauraac wants to merge 26 commits into
Laboratoria:masterfrom
lauraac:master

Conversation

@lauraac

@lauraac lauraac commented Apr 29, 2020

Copy link
Copy Markdown

Code Makers

Alissang and others added 26 commits April 10, 2020 15:05
…ifican, se hace router en index.js pero no cambia a pantalla de registro
…ifican, se hace router en index.js pero no cambia a pantalla de registro
…o the home page. button logout is added. first attempts with firestore
…ection of posts is made but they still cannot be shown
…style. The timestamp is changed to show the post creation date. users are no longer duplicated in firestore
…e are placed but still do not have functionality
…splay user data. function to edit posts does not work yet

@reloadercf reloadercf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Me gusto su proyecto, entiendo la situación del equipo y la falta de una de sus integrantes les genero sobrecarga de trabajo a una de ustedes, aun así considero que pueden complementar con lo faltante como por ejemplo los test, otra cosa que observe en su código es el criterio o ¿en que caso decidieron usar local storage y en que casos usaron firebase? también podrían agregar su archivo Readme para tener en claro más información referente al proyecto.

Comment thread docs/Views/createPost.js
</div>
</header>
<section class="sesionPrincipal">
<form action="" class="newPost">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Si el action del form no tiene nada seria bueno quitarlo

Suggested change
<form action="" class="newPost">
<form class="newPost">

Comment thread docs/Views/createPost.js
</header>
<section class="sesionPrincipal">
<form action="" class="newPost">
<div class="welcome">Hola: <br><br><span id="username"></span></div>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

br no es una etiqueta recomendada, para hacer saltos de linea se recomienda

y si se requiere que sea más espaciado pueden incorporar estilos (margen y padding).

Suggested change
<div class="welcome">Hola: <br><br><span id="username"></span></div>
<div class="welcome">Hola: <p><span id="username" /></p></div>

Comment thread docs/Views/main.js
const loginView = document.createElement('div');
loginView.setAttribute('class', 'viewsStyle');
loginView.innerHTML = `
<center><h3>Inicia Sesión con tu email</h3></center>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Center no es la mejor manera de centrar los elementos, la recomendación es centrar por medio de css.

Suggested change
<center><h3>Inicia Sesión con tu email</h3></center>
<h3 class="h3-center">Inicia Sesión con tu email</h3>

pueden utilizar justify-content: center

Comment thread docs/Views/main.js
<button type="submit" class="registerButton" id="loginButton">INICIAR SESIÓN</button>
<h5><a href="#/passwordRecovery" id="recover">¿Has olvidado la contraseña?</a></h5>
</form>
<div class="conectWithSocialMedia">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

si la clase definida es para implementar estilos, no se recomienda el uso de camel case

Suggested change
<div class="conectWithSocialMedia">
<div class="conect-with-social-media">

<h1>Recupera tu contraseña</h1>
<p>Te enviaremos un correo electrónico para recuperar tu contraseña</p>
<form class="recoveryForm">
<input type="email" required id="loginMail" placeholder= "Correo electrónico">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

espacios

Suggested change
<input type="email" required id="loginMail" placeholder= "Correo electrónico">
<input type="email" required id="loginMail" placeholder="Correo electrónico">

Comment thread docs/firebase/showData.js
Comment on lines +116 to +117
const query = currentUserRef.where('postOwner', '==', localStorage.getItem('uid'));
query.get()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

te sujeto cambiar el nombre de query para especificar bien el nombre de la variable y saber a que hace referencia

Suggested change
const query = currentUserRef.where('postOwner', '==', localStorage.getItem('uid'));
query.get()
const getPosts= currentUserRef.where('postOwner', '==', localStorage.getItem('uid'));
getPosts.get()

Comment thread src/example.js
Comment on lines +1 to +3
// export const example = () => {
// // aquí tu código
// }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

innecesario

Comment thread src/firebase/login.js
Comment on lines +1 to +2
/* eslint-disable no-console */
/* eslint-disable no-alert */

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

si no se utiliza eslint, es mejor desinstalarlo.

Comment thread src/firebase/login.js
const provider = new firebase.auth.GoogleAuthProvider();
firebase.auth().signInWithPopup(provider)
.then(() => {
// const user = result.user;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// const user = result.user;

Comment thread src/styles.css
Comment on lines +148 to +149
width: 85%;
height: auto;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

al definir width y asignarle un valor, puedes omitir height.

Suggested change
width: 85%;
height: auto;
width: 85%;

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