Agregando trabajo grupo Paulina Baeza, Camila Cornejos, María Paz Thompson y Sabrina Villalobos #9
Conversation
There was a problem hiding this comment.
Hola chicas!
Una disculpa enorme por la tardanza, espero no sea demasiado tarde. Por lo menos espero algunos comentarios les sean útiles para aprender aunque no sé si estén aún en tiempo y disposición para hacer cambios.
Veo el app bastante bien, más que nada algunos detalles de formato, pero se entiende qué es lo que están haciendo.
Me parece raro que hayan incluido tantos archivos fuente de otros proyectos, creo que debería haber formas de utilizarlos sin incluir tanto en su proyecto, esto dificultó un poco la revisión, probablemente deberían hablarlo con los profesores/asesores del programa, aunque no sé bien cómo estén trabajando o cuáles son los requerimientos de sus proyectos, entonces tal vez esta es la forma en que debían hacerlo (pero de nuevo, me parece raro, no me imagino que esto esté bien).
No revisé nada de los archivos incluidos en las directorios que parecían de otros proyectos, como Sammy y Stripe, porque creo que nada más fueron copiados de otros repositorios, no? Avísenme si debería revisar algo más.
Algunos comentarios son más para que sepan, pero realmente no es necesario que implementen los cambios porque para fines del proyecto podrían no ser importantes. Ustedes pueden determinar qué cambios creen que sí son necesarios o cuánto tiempo quieren invertir en ello.
| * Copyright 2017 Cina Saffary | ||
| * Licensed under http://opensource.org/licenses/MIT | ||
| * | ||
| * https://github.com/1000hz/bootstrap-validator |
There was a problem hiding this comment.
Este es el mismo archivo que en assets/js ? Alguna razón por la que lo incluyeron dos veces?
| "publishable_key" => "pk_test_6pRNASCoBOKtIshFeQd4XMUh" | ||
| ); | ||
|
|
||
| \Stripe\Stripe::setApiKey('sk_test_BQokikJOvBiI2HlWgH4olfQ2'); |
There was a problem hiding this comment.
Cuando tienen variables como estas que son un string específico, es buena práctica tenerlas sólo en un lugar, asignadas a una variable, y usar esa variable donde las necesiten. En este archivo, por lo menos, podían haber tenido sólo una variable y usarla en los dos lugares donde usan esto.
(Aunque entiendo que esto es código PHP y tal vez no estaba dentro del alcance del proyecto que tenían que hacer)
| firebase.auth().createUserWithEmailAndPassword(emailReg, passReg) | ||
| .then(function() { | ||
| var user = firebase.auth().currentUser; | ||
| user.sendEmailVerification().then(function() { |
There was a problem hiding this comment.
Si no están haciendo nada con la "promesa" que regresa este método, creo que no necesitan escribir el "then" y tal vez incluso tampoco el "catch", aunque probablemente deberían mostrar algún error o algo así, por lo menos agregar algo al log.
Lo mismo para los demás "then" y "catch" vacíos.
| * Copyright 2017 Cina Saffary | ||
| * Licensed under http://opensource.org/licenses/MIT | ||
| * | ||
| * https://github.com/1000hz/bootstrap-validator |
There was a problem hiding this comment.
Esto está bien, pero no era necesario copiar este archivo en su proyecto (A menos que sea una práctica o requerimiento de este programa el no depender de archivos externos, no estoy seguro de qué les hayan requerido).
Este comentario es para que lo sepan, pero no es necesario hacer ningún cambio.
En el proyecto de github de esta librería hay un link que pueden usar con un tag de <script> para importarlo desde un CDN sin tener que copiar el archivo directo en su proyecto (Un Content Distribution Network es básicamente una red de servidores que tienen archivos muy frecuentemente utilizados para que estén siempre accessibles)
Lo mismo podría aplicar para todos los demás archivos que agregaron a su proyecto que son de otras librerías.
| <div class="modal-body-register"> | ||
| <form class="container-fluid" data-toggle="validator" role="form"> | ||
| <div class="form-group"> | ||
| <div class="row"> |
There was a problem hiding this comment.
Hay una línea con clase "row" que envuelve a otras "rows", esto fue a propósito? Tal vez no necesiten esta envolviendo a las demás, o tal vez alguna no cerró como es debido.
| } | ||
| /* | ||
| * ruta basica #, cuando el navegador está en #, la | ||
| * funcion que definimos se ejecutará en un contexto sammy |
There was a problem hiding this comment.
Creo que debería haber un punto al final de esta línea y tal vez comenzar la siguiente con mayúscula.
Es un detalle muy pequeño, pero se lee como si fuera una continuación de la misma oración (como están haciendo en la línea anterior) y puede ser confuso.
|
|
||
| #header { | ||
| background: rgba(68,102,154,1); | ||
| background: -moz-linear-gradient(top, rgba(68,102,154,1) 0%, rgba(85,126,187,1) 100%); |
There was a problem hiding this comment.
La indentación en este archivo también está mal.
| @@ -0,0 +1,11 @@ | |||
| <div class="item col-md-3"> | |||
There was a problem hiding this comment.
De nuevo la indentación. Por qué hay espacios aquí al inicio?
Me imagino tal vez producto de hacer copy-paste, pero es importante mantener un formato correcto para que el código sea legible. Estoy seguro pueden encontrar herramientas en línea que les ayuda a darle formato a su código html.
| <div class="item-detail"> | ||
| <div class="col-md-6"> | ||
| <div class="row"> | ||
| <div class="back-link"><a href="#/">« Back to Items</a></div> |
| })(jQuery); | ||
|
|
||
| /* | ||
| * botón de pagp por Stripe, se puso en item_detail.template |
There was a problem hiding this comment.
Esto quiere decir que se paga cara artículo por separado?
No description provided.