Alvax10 / Lost_pets_react

0 stars 0 forks source link

Review #6

Open Marcosreuquen opened 2 years ago

Marcosreuquen commented 2 years ago

Buenas, Alvax!! Estuve viendo tu desafío ayer y me quedé con algunas cositas para dejarte acá.

Lo primero que me pasó fue que no pude modificar mis datos. Lo probé en dos momentos diferentes. Primero me dio que no existía PUT a esa ruta y la segunda me lo rechazó por internal server error. En la segunda vi que era posible sólo cambiar el password. image image

Después reporté una mascota y me dio otro error. También pareciera del server se rompe en algún momento del flujo. De todas formas, en este momento del flujo me di cuenta que los endpoints requieren un email pero el mismo no se guarda en los atomos o el storage que se use. Tal vez el uso del mail se podría reemplazar por el token. O quitarlo, sobre todo en la parte de reportar mascotas si es que se piensa para que lo reporte cualquier usuario loggeado o no. image image

Lo mismo del mail me sucedió cuando quise acceder a mis mascotas para editarlas. No me las mostró por la falta del mail. image

Por otro lado, tema código. En hooks.ts tal vez podrías separar los mismos hooks de los atoms para escalar más prolijamente el módulo. O convendría no exportar los átomos para no exponer esa capa.

Sobre los buttons, podrías evitar la repetición de los botones pasandolé el color que quieras utilizar por props para hacelo más reutilizable y ahorrar el peso de las importaciones cuando usas muchos en la misma page.

Como sugerencia, sobre la page montada en / sólo toma la ubicación. Podrías evitarte ese paso con un hook y darle la page home como entrada. Digo esto por dos cositas, si el user no da la ubicación no es 100% excluyente para el uso de la app, puede querer publicar una mascota y ahí tenés el mapa. Lo otro, la página de entrada debería darte información de la página como buena práctica. No es un must-have de este desafío pero está bueno para tenerlo en cuenta.

Y lo último, también enfocado en las buenas prácticas, tenés dos o tres componentes mezclados en algunas carpetas. Por ejemplo Home+Card Teniendo en cuenta que Card puede ser un ui y home una page, podría llegar a ser confuso al momento de buscar alguno de los dos módulos para reutilizarlo o para arreglar algo en ellos. En esto también podrías dentro de las carpetas trabajar sólo algo en particular pero tener tu módulo principal (el que exporta el componente) como index para facilitar la importación.

Estos últimos temas son más para ir puliendoló. Dejo el desafío como "necesita ajustes" para arreglar el flujo que se corta por los bugs con los endpoints.

Saludos!

Alvax10 commented 2 years ago

Buenas marcos! gracias por los tips!

Ahora me pongo a ver el tema de porqué no se guarda el email, pero mi idea era que solo personas logueadas puedan reportar mascotas así si una persona reporta que vió a esa mascota perdida, se le puede mandar un mail con la data.

Lo de cambiar tus datos solo se me había ocurrido cambiar la contraseña porque es lo uniuco que los usuarios cambian en general, pero voy a añadir el nombre y el email para modificar también!

Voy a sacar de Home la card y la voy a poner en una carpeta de componente aparte. Y también voy a ver si me puedo "saltear" la parte de la ubicación para que no aparezca siempre, o capaz ahí añadir la data de lo que hace la página, pero aclaro que no lo hice porque en el diseño de figma no lo hace.

Muchas gracias marcos! saludos

Marcosreuquen commented 2 years ago

Buenas, Alva. Estuve probandoló de nuevo y se rompía el back en el update de la mascota. Me cloné tu repo para probar qué podría estar pasando y lo probé con mi back.

Te dejo los detalles que vi:

Y por último, un detallito más como sugerencia. En algunos componentes haces uso de hooks en los que no necesitas el seter. En estos retornas el useRecoilState. Tal vez podrías usar useRecoilValue para evitar errores de uso en componentes que no deberían tener ese poder.

https://github.com/Alvax10/dwf-m8-desafio/blob/5a00243e423eab8a0d8e71f50f7c6dcaca859c21/src/components/Home/Home.tsx#L14-L17

Y cualquier cosa etiquetame en discord así lo vemos! Saludos.

Alvax10 commented 2 years ago

Buenas, marcos! Gracias por tomarte el tiempo, yo borré recoil porque la consola me tiraba un error de que ya estaba instalado en otra dependecia y me anduvo bien así.

Dale, voy a agregar una alerta o algo para que se sepa.

Vos sabes que a mi si me modifica la data, me lo hace bien, cuando yo uso la página no me tira errores, por eso no puedo identificar este tipo de cosas, pero voy a checkear bien, a ver que puede estar pasando. Gracias!

El mié., 16 de marzo de 2022 8:00 p. m., Marcos Reuquén < @.***> escribió:

Buenas, Alva. Estuve probandoló de nuevo y se rompía el back en el update de la mascota. Me cloné tu repo para probar qué podría estar pasando y lo probé con mi back.

Te dejo los detalles que vi:

  • En el package falta recoil como dependencia. Si bien está recoil persist, usas métodos propios de recoil que lo necesitan (eso es lo que rompía cuando no guardaba el mail en un átomo);

  • Las llamadas a la api en send-email-to-user y update-mascot-info no obtienen respuesta del back. Puede ser que el back esté resolviendo el pedido sin dar respuesta. (no llega tampoco un 5**)

  • send-email-to-user: Me llegó el mail pero el front no obtuvo respuesta. La UI podría mostrar un alert o algo similar donde te diga que salió todo bien o todo mal.

  • update-mascot-info: este caso puede ser un poco más complejo, porque no devuelve respuesta ni tampoco cambia los datos de la mascota. Estaba viendo en tu back y puede ser que el timeout de cloudinary sea muy amplio, que se modifique en postgres pero no en algolia o mismo algo en el endpoint que no tira al controller lo que tiene que hacer. Te diría que le metas try/catch a toda esa parte del flujo desde local y vayas viendo donde se rompe.

Y por último, un detallito más como sugerencia. En algunos componentes haces uso de hooks en los que no necesitas el seter. En estos retornas el useRecoilState. Tal vez podrías usar useRecoilValue para evitar errores de uso en componentes que no deberían tener ese poder.

https://github.com/Alvax10/dwf-m8-desafio/blob/5a00243e423eab8a0d8e71f50f7c6dcaca859c21/src/components/Home/Home.tsx#L14-L17

Y cualquier cosa etiquetame en discord así lo vemos! Saludos.

— Reply to this email directly, view it on GitHub https://github.com/Alvax10/dwf-m8-desafio/issues/6#issuecomment-1069723688, or unsubscribe https://github.com/notifications/unsubscribe-auth/APKSAAUAQ6BRHXLVW7D4GULVAJRX7ANCNFSM5QHVEHJQ . You are receiving this because you commented.Message ID: @.***>