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

Feedback #23

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

Feedback #23

wants to merge 20 commits into from

Conversation

keupa
Copy link

@keupa keupa commented Mar 13, 2020

No description provided.

keupa and others added 20 commits February 26, 2020 09:07
Se crearon tres historias de usuario que se desarrollaron a través de tres sprints: El usuario podrá desplegar y escoger a través de un <select>, la lista de países y el indicador deseado. Se desplegará la información de los años y porcentaje en una tabla HTML. El usuario podrá seleccionar un país, un indicador y distintos años a través de ventanas modales. Posteriormente se desplegará la información sólo de los años elegidos en una tabla HTML. El usuario podrá seleccionar un país, un indicador y distintos años a través de ventanas modales. Además podrá elegir entre desplegar la información en una tabla o gráfica.
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.

Muy bien la semántica y los nombres de funciones, agregar comentarios más descriptivos en los commit, lamentablemente solo pude ver tu trabajo, el de tu compañera no es visible, muy bien por el producto final.

console.log(indicator)
}

function loadYears (list) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bien por el nombre de las funciones en ingles

Comment on lines +84 to +98
/*function setYearOptions(list) {
list.forEach(i=>{
let option = document.createElement('option')
option.value = i
option.label = i
yearSelect.appendChild(option)
})
}

function yearOnChange(e){
let value = e.target.value
year = value
console.log(year)
}
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Evitar dejar código comentado, mejor eliminar

})

function openModal(modal){
if(modal==null) return
Copy link
Contributor

Choose a reason for hiding this comment

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

if(!modal) esto equivale a decir: if(modal===null || modal===undefined) y es mejor ;)

test.addEventListener('click', onChange)
indiSelect.addEventListener('change', indiOnChange)
overlay.addEventListener('click', ()=>{
const modals = document.querySelectorAll('.modal.active')
Copy link
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 qurySelector

overlay.addEventListener('click', ()=>{
const modals = document.querySelectorAll('.modal.active')
modals.forEach(modal=>{
closeModal(modal)
Copy link
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 una función para no repetir código

display: none;
}

label{
Copy link
Contributor

Choose a reason for hiding this comment

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

Evitemos editar etiquetas, preferible clases

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.

2 participants