-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pull request #24
base: master
Are you sure you want to change the base?
Pull request #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Los comentarios en los commits pueden mejorar mucho, "modifique mani.js" por ejemplo no dice nada, no es util el comentario, y eviten el español a toda costa, muy bien el trabajo progresivo y en equipo, pero deben arreglar el usuario para que no aparezca gris, cómo anónimo. Muy bien por el proyecto y el uso de fetch!
left: 0; | ||
width: 560px; | ||
height: 315px; | ||
top: 1580px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El usar pixeles fijos, evita que sea responsivo
@@ -53,5 +53,5 @@ function traer() { | |||
}); | |||
}); | |||
} | |||
|
|||
window.onload = traer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esto no es necesario si el script está hasta el final de body, eviten las funciones en español
@@ -66,7 +66,7 @@ <h2>SHOW ME WHAT YOU GOT</h2> | |||
</article> | |||
<article class= "inter"> | |||
<div> | |||
|
|||
<iframe width="560" height="315" src="https://www.youtube.com/embed/V6SfEIoEHY0" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Los iframes se evitan a toda costa, pueden crear huecos de seguridad
@@ -72,6 +72,8 @@ Adriana nos apoyó con el arreglo de la maqueta para que los datos no se ocultar | |||
|
|||
## 8. Prototipo final | |||
|
|||
<img src="https://github.com/AndyyAg/CDMX009-Data-Lovers/blob/master/images/prototipofinal.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien por el uso de imagenes locales
@@ -90,46 +90,46 @@ visualizar y manipular data. | |||
|
|||
### UX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El usuario configurado en global de la termina no coincide con el usuario de Github por eso aparece así en gris y sin foto
color: #ffffff; | ||
} | ||
|
||
.modal-window header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien los nombres de las clases
src/main.js
Outdated
div = appendChild('div'); | ||
img.src = character.image; | ||
characters.innerHTML = `${character.name}`; | ||
append(li, img); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien por el uso de funciones para evitar repetición de código
src/main.js
Outdated
img.src = character.image; | ||
span.innerHTML = `${character.name}`; | ||
characters.innerHTML = `${character.name}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si no se mezcla text y variables no es necesario usar ``podría ser solo characters.innerText = "character.name"
@@ -42,10 +44,10 @@ function traer() { | |||
fetch(api + input.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excelente por el uso de la API
:)