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

Feature: Add Svg component #34

Closed
wants to merge 10 commits into from
Closed

Feature: Add Svg component #34

wants to merge 10 commits into from

Conversation

Zyruks
Copy link

@Zyruks Zyruks commented Apr 8, 2024

Cambios Realizados 🎉

  • feat: introduce el componente Svg para la renderización unificada de SVGs

Descripción de los Cambios

Se ha introducido un nuevo componente Svg para centralizar la renderización de SVGs en toda la aplicación, mejorando la mantenibilidad y escalabilidad del código. Los puntos clave incluyen:

  • El componente Svg soporta la renderización dinámica de SVG mediante IconCatalog y IllustrationCatalog, permitiendo una gestión más flexible y escalable de iconos e ilustraciones.
  • Se refactorizaron componentes existentes (Home y Header) para utilizar el nuevo componente Svg, reemplazando las importaciones directas de SVG y los SVGs en línea.
  • Se aseguró una apariencia y estilización consistentes de los SVGs en diferentes contextos integrando el componente Svg con propiedades className adecuadas para ajustes de tamaño y color.

Esta nueva característica simplifica el proceso de gestión y renderización de SVGs, contribuyendo a un código más limpio y mantenible.

Visuales (Opcional)

Original

image

Actualizado

image

Como usarlo

Code_April_08_2024_FQYf.mp4

Comparación con los componentes del folder Icons/
Screen Shot 2024-04-08 at 16 01 56

Lista de Verificación ✅

  • Mi código sigue el estilo de código de este proyecto.
  • Mi cambio requiere un cambio en la documentación.
  • He actualizado la documentación acordemente.

@fforres
Copy link
Member

fforres commented Apr 9, 2024

Holas @Zyruks

Referente a este y a #35 este PR.

Son cambios bien core en la estructura del proyecto, y que requieren conversación con el equipo de desarrollo, conversaste el proceso o la idea con alguien antes de hacerlos? O quizas hay algun issue/conversación de discord asociadas que no vi? 🤔

@fforres fforres mentioned this pull request Apr 9, 2024
5 tasks
Zyruks added 10 commits April 9, 2024 10:57
…ribute handling

- Introduced SVGPathAttributeNames enum to cover all possible SVG path
attributes, ensuring a more robust and error-free attribute handling.
- Defined PathOption and SvgPath interfaces to streamline the creation
and manipulation of SVG paths with optional attributes.
- Expanded SvgType interface with viewBox and paths to encapsulate the
complete structure of an SVG, facilitating easier rendering and manipulation
of SVG graphics in the project.
- Introduced `Icon` component for dynamically rendering SVG illustrations
based on the `IllustrationCatalog`.
- Added `IllustrationCatalog` enumeration and `IllustrationMapping`
for mapping illustration names to their SVG path data and attributes,
facilitating easy addition and management of illustrations.
- Implemented utility functions within the `Icon` component to convert
illustration options to SVG attributes, enhancing flexibility in SVG
customization.
… into Svg

- Removed `Icon.tsx` and `Illustration.tsx` components, introducing `Svg`
  for unified SVG rendering, enhancing maintainability.
- Integrated `IconCatalog` and `IllustrationCatalog` with `Svg`, enabling
  dynamic rendering from a unified `catalog` prop.
- Implemented conditional rendering in `Svg` to distinguish between icon and
  illustration types, improving component flexibility.
- Preserved and refactored utility functions for converting attributes and
  rendering paths, ensuring compatibility with existing structures.

This refactor merges functionalities into a versatile component, reducing
redundancy and facilitating future SVG handling enhancements.
…tration rendering

- Replaced usage of the `Mountain` component with the `Svg` component to
  render the mountain illustration on the home page. This change leverages
  the newly integrated `Svg` component, enhancing consistency and
  maintainability in SVG rendering.
- Updated imports to include `Svg` and `IllustrationCatalog` from the
  `Catalogs` directory, aligning with the project's new structure for handling
  SVGs.
- Utilized `IllustrationCatalog.mountain` as the `catalog` prop for the `Svg`
  component to dynamically render the mountain illustration, simplifying the
  SVG management process.

This update signifies a step towards unifying SVG rendering mechanisms across
the project, streamlining the codebase and improving flexibility in handling
illustrations and icons.
- Removed direct use of the `Logo` component in favor of the `Svg` component
  to render the logo within the `Header`. This adjustment aligns with the
  broader strategy of consolidating SVG rendering mechanisms.
- Imported `IconCatalog` alongside `Svg` to facilitate dynamic referencing of
  the logo via `IconCatalog.logo`, enhancing flexibility and scalability in
  icon management.
- Adjusted logo size through the `className` prop to ensure consistent
  appearance, adapting to the `Svg` component's styling approach.
@joseglego
Copy link
Member

Hola @Zyruks , @fforres ya te respondió pero no he visto más movimiento.

Estoy intentando retomar este repo y quiero darte mi opinión (MUCHA énfasis en mi opinión y que puede estar sesgada)

Como comentó @fforres lo ideal es discutir antes de hacer cambios. Siento que todos los cambios que hiciste fueron muy amplios y cambian mucho la arquitectura lo que podría ser un problema de llevar el código a prod y cómo intentamos que vayan las cosas en el proyecto.

Lo primero, a nivel de código se ve muy genial y ordenado! Eso me gustó. Hay mejoras sobre lo que tenemos, pero también hay cosas que podrían complicarse o que no es el enfoque por el que lo intentabamos llevar.

Respecto a este PR inicial, tengo los siguientes comentarios:

  1. Me gusta
  • La definición del componente: src/components/Svgs/Svg.tsx es una buena manera de hacerlo.
  • Está genial poder pasarle className, etc.
  • Está genial poder importar todo desde Icons y ya
  1. No me gusta cómo le pasas los componentes en el param catalog usando src/components/Svgs/Catalogs/IllustrationCatalog.ts,

Por qué 2? Me gusta más mantener los SVG por separados. Es más limpio, fácil de mantener/soportar/controlar. Además de optimo en código (muy nit pick). De hecho, he visto que lo más común es tener los svg por separados. Como Lucide, Material o Feather

Entonces, si me gustaría hacer merge de esto pero con el approach de los svg por separados. Me gustaría saber qué opinas, si quieres discutirlo o si ya no estás interesado. disculpa la respuesta demorada. Quizás yo puedo usar tu branch como base. etc.

(Por esto te tipo de discusiones es que comentamos para juntarnos primero, porque has hecho trabajo que podría llegar o no a main, lo siento)

Podemos juntarnos o chatear por Discord.

@Zyruks Zyruks closed this by deleting the head repository Jun 23, 2024
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.

3 participants