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

CodeReview#26

Open
EtnaSI wants to merge 66 commits into
Laboratoria:masterfrom
EtnaSI:master
Open

CodeReview#26
EtnaSI wants to merge 66 commits into
Laboratoria:masterfrom
EtnaSI:master

Conversation

@EtnaSI

@EtnaSI EtnaSI commented Mar 14, 2020

Copy link
Copy Markdown

Falta depurar código y hacer pruebas.

@HectorBlisS HectorBlisS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Muy bien las descripciones en los commits, lastima que están en español, evítenlo en el siguiente proyecto. Se nota el trabajo progresivo y duro y también el trabajo en equipo, felicidades por el producto final, muy completo.

Comment thread test/data.spec.js

it('should return Bulbasur first and Ivysaur after', () => {
expect(pokemon).toHaveProperty('weaknesses', 'Ice');
expect(pokemon.id).toBe('1' , '2');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

muy bien por los tests

Comment thread src/data.js
// console.log(pokemon);

const data = {
saveNumber: (searchNumber) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Muy bien los nombres de las funciones en ingles

Comment thread src/data.js
//console.log(pokemon);
// console.log(pokemon);

const data = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No es necesario crear un objeto, se pueden exportar las funciones e forma independiente

Comment thread src/data.js
Comment on lines 42 to 45
orderFilter = pokemon.sort((a, b) => {
if (a.name > b.name) {
return 1;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Se entiende porque se hace esto evitando el ordenado default unicode?

Comment thread test/data.spec.js
@@ -1,5 +1,5 @@
// importamos la función `example`
import { data } from "../src/data";
import { data } from '../src/data';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

XD

Comment thread src/style.css
@@ -0,0 +1,234 @@
@import url('https://fonts.googleapis.com/css?family=Saira+Condensed&display=swap');

Copy link
Copy Markdown
Contributor

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 fuentes externas

Comment thread src/main.js Outdated
Comment on lines +133 to +134
let overlay = document.querySelector('#profileContainer');
let popup = document.getElementsByClassName('profileCard');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preferible usar querySelector, ya que getElementsByClassName devuelve un tagList (lista extraña que no es un array)

Comment thread src/main.js Outdated
//console.log(newPopup);
let btnClosePopup = document.getElementsByClassName('btn-cerrar-popup')
let btnClosePopup = document.getElementsByClassName('btn-cerrar-popup');
let newBtnClose = btnClosePopup[btnClosePopup.length-1];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Si buscamos un lemeno en especifico es mejor darle un id

Comment thread src/main.js Outdated
Comment on lines 292 to 293
overlay.classList.remove('active')
newPopup.classList.remove('active')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bien el uso de classList

Comment thread src/main.js Outdated
Comment on lines +480 to +483
const profileContainer = document.createElement("div");
profileContainer.className = "profileContainer";
profileContainer.id = "profileContainer";
document.body.appendChild(profileContainer);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Muy bien

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.

3 participants