geekha/composite-header

[geekhadev]

• Se crea el componente Header para simplificar un poco la página principal
• Se crea el componente HeaderLink para simplificar el componente Header
• Se simplifica un poco la sintaxis de los links:

{link.id == "linkedin" ? (
  <Linkedin strokeWidth={1} />
) : null}

a este cambio:

{link.id == "linkedin" && <Linkedin strokeWidth={1} />}

[joseglego]

Hola 👋

Disculpa la demora! Buenas recomendaciones 👍

La idea de separar el Header es algo que teníamos mapeado pero no habíamos podido hacer por temas de tiempo y porque no se había complicado "tanto" aún.

El tema de los HeaderLinks también nos parece genial, pero es un poco más complicado llegar a un acuerdo.

Tengo 2 comentarios al respecto:

1. Link es MUY subjetivo
   A. En vista de que HeaderLink esta solo asociado a Header entonces sería mejor tener una carpeta en components Header/ y allí podríamos tener Header.tsx y HeaderLink.tsx. De esta manera mantenemos la lógica y scope juntos. No hay ejemplos porque la página aún está chica y no habíamos comenzado a separar en componentes.
   B. Podríamos definir un component Link.tsx general para la app que también vamos a necesitar. Y le pasamos como parametro adicional el className. y Así queda inclusive más reusable par atodos.

Las 3 aproximaciones son correctas. Todas tienen sus pros y contras. Por eso te comento que es subjetivo. Sería mejor coordinar para llegar a un concenso.

2. En lo personal (mucho enfásis en lo personal, porque es subjetivo), yo siempre prefiero usar ? sobre && para el render. Ya que a pesar de que podría ser más legible podría dar errores en ciertos casos (en este no, pero prefiero usar siempre el mismo estilo).
   Referencia: https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx.
   OJO: no sé si la referencia sigue aplicando en 2024 pero es la razón de porqué el código estaba así.

En conclusión:

• Son buenas recomendaciones y está genial
• El 1. Está genial pero sería mejor llegar a un conceso antes.
• El 2. Es algo subjetivo en este caso, pero preferiría no cambiarlo en próximos PRs o deshacerlo.

Basados en que son buenas mejoras, nos parece genial mergearlo y discutimos próximos pasos en un nuevo PR :)

Muchas gracias por tu ayuda.

[joseglego]

Al intentar mergearlo, da error:

[warn] Code style issues found in 3 files. Run Prettier to fix.
Error: Process completed with exit code 1.

Podrías solucionar los errores para mergearlo?

[irwingnaranjo]

Hola @joseglego lo voy a revisar y hacer la adaptación.

[joseglego]

Hola @irwingnaranjo 👋

Toma en cuenta que el único cambio necesario para el merge es el error del linter. Lo demás era solo feedback y se puede discutir a futuro.

Nuevamente, muchas gracias.

[geekhadev]

Hola @joseglego he realizado los cambios adaptaciones del estilo de código y creé una carpeta Header para mantener el scope de los componentes organizado.

Gracias por el consejo de la evaluación && no es correcto hacerlo así porque los valores no son los mismos. Seguiré componetizando lo que pueda y apoyando, te he escrito por discord precisamente para ponernos de acuerdo antes de ponerme a hacer cosas. 👋

[joseglego]

Ohhhh muchísimas gracias! No era necesario porque cada aporte cuenta! Y podíamos dejarlo como próximo PR! Pero genial!

Mea culpa lo de Discord! tienes toda la razón!