Conversation
UlisesGascon
left a comment
UlisesGascon
left a comment
There was a problem hiding this comment.
@codemcu, como te dije en #39. Estas en otro nivel y el grado de exigencia es mayor. Dicho esto, el código es elegante y funciona bien, aunque tienes un par de problemas en el HTML. (título del documento y gestión de imágenes transparentes). Me gusto mucho que incluyeras el Score
Dos cosas importantes... DRY y KISS. Estas haciendo dos llamadas AJAX para averiguar los followers y followings... cuando realmente lo podrías sacar la información haciendo una única llamada a su perfil https://api.github.com/users/ y evitando que rápidamente sobrepase el límite.
Espero un refactor 🕶
mauricio/githubSearch/css/style.css
Outdated
| background: #355C7D"*/ | ||
|
|
||
| body { | ||
| /*background-color: #F8B195;*/ |
There was a problem hiding this comment.
ℹ️ El código comentado... sobra 
mauricio/githubSearch/css/style.css
Outdated
| width: 620px; | ||
| margin: 0 auto 0; | ||
| list-style-type: none; | ||
| /*background-color: #355C7D;*/ |
There was a problem hiding this comment.
ℹ️ El código comentado... sobra
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| } else { | ||
|
|
||
| if (document.querySelector('ul') !== null) { |
There was a problem hiding this comment.
Mete este selector en una variable
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| loadAjax(`https://api.github.com/search/users?q=${text}+in%3Afullname&type=Users`) | ||
| .then(getNetwork) | ||
| .catch(error => console.log(error)); |
There was a problem hiding this comment.
Gestiona el error de forma visual también
There was a problem hiding this comment.
he metido try/catch pero como console.log, por ahora...
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| if ($event.code === 'Enter' && $event.isTrusted) { | ||
|
|
||
| if (input.value.trim() === '') { |
There was a problem hiding this comment.
No necesitas === "" podrías hacer !input.value.trim() o simplemente input.value.trim() que sería más legible
mauricio/githubSearch/js/main.js
Outdated
| const p = document.createElement('P'); | ||
| container.appendChild(p); | ||
| p.textContent = 'Please enter username'; | ||
| return false; |
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| users.forEach(function (item) { | ||
|
|
||
| const followers = new Promise( (resolve, reject) => { |
There was a problem hiding this comment.
followersy followings son lo mismo con distintas urls. DRY!
There was a problem hiding this comment.
Nadie gestiona los errores... 🤦♂️
mauricio/githubSearch/js/main.js
Outdated
| getFollowings(followings, div); | ||
| } | ||
|
|
||
| function getFollowers (followers, div) { |
There was a problem hiding this comment.
getFollowers y getFollowings son lo mismo con distinto texto y target.... DRY!
There was a problem hiding this comment.
async/await al rescate!. Aparte de mi despiste de followers y following en /user
UlisesGascon
left a comment
UlisesGascon
left a comment
There was a problem hiding this comment.
Feedback
- Muy bien por usar
async/await - El código está algo descuidado, pero nada que un buen reactor no ayude a solucionar
- Me gusto mucho la idea del buscador! 👏
- Estaría bien apoyar un poco al usuario cuando se esperan datos o no tenemos información que mostrar xD
🌟 Me gusta el diseño y que aparezcan todas las coincidencias!
Bugs
- Qué pasa cuando alcanzas el límite?
- No hay spinners ni similar...
mauricio/githubSearch/js/main.js
Outdated
| reject(response.json()); | ||
| } | ||
| }) | ||
| .catch(error => console.log(error)); |
There was a problem hiding this comment.
Aqui haces un return raro...
mauricio/githubSearch/js/main.js
Outdated
| input.addEventListener('keyup', searchUser, false); | ||
| const container = document.querySelector('.container'); | ||
|
|
||
| async function searchUser($event) { |
There was a problem hiding this comment.
el naming de $event fuera de angular.. no es muy extendido ;-)
mauricio/githubSearch/js/main.js
Outdated
| @@ -0,0 +1,112 @@ | |||
| (function () { | |||
mauricio/githubSearch/js/main.js
Outdated
| try { | ||
| if ($event.code === 'Enter' && $event.isTrusted) { | ||
|
|
||
| if (!input.value.trim()) { |
There was a problem hiding this comment.
Esto lo reutilizas luego como text..
| document.querySelector('ul').remove(); | ||
| } | ||
|
|
||
| const text = input.value.trim(); |
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| } else { | ||
|
|
||
| const isUl = document.querySelector('ul') !== null; |
There was a problem hiding this comment.
es raro como manejas este condicional y su contexto con document.querySelector('ul') e isUl
mauricio/githubSearch/js/main.js
Outdated
|
|
||
| const resArray = data.items; | ||
|
|
||
| resArray.forEach(async function (item) { |
There was a problem hiding this comment.
no es mas sencillo un Promise.All()?
2 participants
No description provided.