Skip to content
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

Mariel y Karina #34

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

marielcarrillo
Copy link

No description provided.

Copy link
Contributor

@HectorBlisS HectorBlisS left a comment

Choose a reason for hiding this comment

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

buen trabajo, hay que mejorar las descripciones de los commits. =)

src/data.js Outdated
export const allCharacters = data.results

//exportar todos los Ricsk´s
export const allRicks = data.results.filter(value =>(value.name.includes ("Rick")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy buenos filtros

@@ -3,8 +3,12 @@ import data from './data/rickandmorty/rickandmorty.js';
//funcion para encontrar desde la data a los 5 personajes principales
export const principalCharacters = data.results.filter(char =>
(char.id==1||char.id==2||char.id==3||char.id==4||char.id==5))
Copy link
Contributor

Choose a reason for hiding this comment

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

Esta concatenación es muy buena, solo agregar el tercer =

src/main.js Outdated
Comment on lines 37 to 41
let boxCharacters = item.image;
let nameCharacter = item.name;
let name = document.createElement('p');
let img = document.createElement('img');
let btn = document.createElement('button');
Copy link
Contributor

Choose a reason for hiding this comment

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

Podriamos sustituir estos nodos por <p> <img/> <button></button> </p> con "backtics"

src/main.js Outdated
Comment on lines 48 to 55
name.style.fontFamily = "kindergarten";
name.style.fontSize = "20px";
name.style.color = "white";
btn.style.border = "none";
btn.style.background = "0,0,0,0.0"
btn.style.width = "20em";
btn.style.height = "20em";
img.style.width = "14em";
Copy link
Contributor

Choose a reason for hiding this comment

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

Es mucho mejor usar una clase solamente en vex de estilo por estilo

src/main.js Outdated
Comment on lines 2 to 5
console.log(principalCharacters)
console.log(allCharacters)
/*/funcion recorriendo (swicht)
const imprimirResultado = document.querySelectorAll('btnPrincipal').addEventListener('click', );

switch(imprimirResultado) {
case btnRick:
document.querySelector('#rickPrincipal').addEventListener("click", btnRick);
break;
}*/

//ésta funcion me presneta los datos del personaje 1 que se encuentra en la posición [0]
document.querySelector('#rickPrincipal').addEventListener("click", btnRick)

function btnRick (){

document.querySelector("#idName").innerHTML =
`<li>Nombre: ${(principalCharacters[0].name)}</li><br>
<li>Status: ${(principalCharacters[0].status)}</li><br>
<li>Especie: ${(principalCharacters[0].species)}</li><br>
<li>Genero: ${(principalCharacters[0].gender)}</li>`
};

document.querySelector('#mortyPrincipal').addEventListener("click", btnMorty)

function btnMorty (){
document.querySelector("#idName").innerHTML =
`<li>Nombre: ${(principalCharacters[1].name)}</li><br>
<li>Status: ${(principalCharacters[1].status)}</li><br>
<li>Especie: ${(principalCharacters[1].species)}</li><br>
<li>Genero: ${(principalCharacters[1].gender)}</li>`
console.log(allRicks)
console.log(allMortys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Usen todos los console.log necesarios para desarrollo pero intenten no ubirlos al repo ;)

Comment on lines +37 to +41
`<li>Nombre: ${(principalCharacters[2].name)}</li><br>
<li>Status: ${(principalCharacters[2].status)}</li><br>
<li>Especie: ${(principalCharacters[2].species)}</li><br>
<li>Genero: ${(principalCharacters[2].gender)}</li>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy bien

Comment on lines +66 to +71
let screen = document.querySelector(".principalScreen")
screen.style = "display:none";
document.querySelector(".screenCharacters").style.display="block";
}
document.querySelector(".btnAllCharacters").addEventListener("click", screenCharacters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Los nombres de funciones un poco más descriptivos

src/index.html Outdated
Comment on lines 24 to 26
<img src="images/btnAllCharacters.png" alt="" class="btnAllCharacters">
<img src="images/btnRicks.png" alt="">
<img src="images/btnMortys.png" alt="">
Copy link
Contributor

Choose a reason for hiding this comment

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

Intenten ser más consistentes con las etiquetas si arriba se usaron a, aquí podrian haberlas usado también.

@@ -6,6 +6,7 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
Copy link
Contributor

Choose a reason for hiding this comment

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

Avences viernes no es un comentario de commit descriptivo, intenten ser semánticas en todo momento ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants