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

feat: Implement app theme handling #8

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

Conversation

lazaroysr96
Copy link
Collaborator

@lazaroysr96 lazaroysr96 commented Nov 24, 2024

Realizando Optimización General.

En este PR estaré revisando el código y resolviendo detalles respecto a componentes que se deban mejorar.

Cambios realizados

  • Se solucionó problema con el calendario, anteriormente los días de la semana no se mostraban adecuadamente.
  • Se añadieron funciones para habilitar modo oscuro en el futuro.
  • Se mejoró el input para modo oscuro.

Copy link

netlify bot commented Nov 24, 2024

Deploy Preview for erc20-vault failed.

Name Link
🔨 Latest commit 2146611
🔍 Latest deploy log https://app.netlify.com/sites/erc20-vault/deploys/6768311f5a64ae0008eaa412

@lazaroysr96 lazaroysr96 self-assigned this Nov 24, 2024
Copy link
Collaborator

@criss8X criss8X left a comment

Choose a reason for hiding this comment

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

En lugar de parsear un booleano, y convertirlo a string, puedes usar un string fijo asi evitas parseos innecesarios, puedes poner como dato a guardar, dark si es modo oscuro, o light como modo claro y en caso de no estar ninguno pos el por defecto, tambien usaras constantes.

const DARK_MODE = "dark";
const LIGHT_MODE = "light"

src/hooks/useTheme.ts Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
@Ariel11R Ariel11R closed this Dec 8, 2024
@lazaroysr96 lazaroysr96 reopened this Dec 8, 2024
@lazaroysr96 lazaroysr96 marked this pull request as ready for review December 8, 2024 16:37
@lazaroysr96 lazaroysr96 requested a review from criss8X December 8, 2024 16:38
- Se solucionó problema que impedía que se cargara la página, no se estaba obteniendo el TEST_TOKEN_SEPOLIA adecuadamente.
Copy link
Collaborator

@criss8X criss8X left a comment

Choose a reason for hiding this comment

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

image

El calendario tiene bug los botones

src/components/ButtonSwitchTheme.tsx Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
@criss8X criss8X changed the title feat: General code optimization feat: Implement toggle theme Dec 12, 2024
@criss8X criss8X changed the title feat: Implement toggle theme feat: Implement app theme handling Dec 12, 2024
@lazaroysr96 lazaroysr96 requested a review from criss8X December 15, 2024 14:36
Copy link
Collaborator

@criss8X criss8X left a comment

Choose a reason for hiding this comment

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

Hay un Bug visual en el boton de el calendar de seleccionar.

Como deberia estar:
image

Como se ve:

image

image

src/components/ButtonSwitchTheme.tsx Outdated Show resolved Hide resolved
src/components/ButtonSwitchTheme.tsx Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
src/hooks/useTheme.ts Outdated Show resolved Hide resolved
@lazaroysr96 lazaroysr96 requested a review from criss8X December 22, 2024 15:38
Copy link
Collaborator

@criss8X criss8X left a comment

Choose a reason for hiding this comment

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

El calendar lo arreglaste perfectamente, ahora hay cosas a tener en cuenta:

ButtonSwitchTheme:

  1. Cambiarle el nombre a SwitchTheme unicamente, si el boton switch theme es el unico que cambia de tema, entonces en lugar de crear un hook para el puedes establecer la logica dentro de Switch Theme, va ser mejor.
  2. Deberias poner el boton en algun lado, si esta creado debe implementarse en algun sitio. Busca algun sitio sin que rompa con la interfaz en todas las pantallas, para eso deberas usar las herramientas de desarrollo.
  3. Debes implementarle una animacion sencilla para darle un toque interesante, hay muchas plataformas que muestran animaciones para cambios de tema puedes usar framermotion.
  4. El uso de literales sin usar la constante:
theme === "dark" ? <> </> : <></>

ahi estas poniendo "dark" es cierto que ese valor no cambiara, pero es una mala practica hacer eso porque normalmente trabajamos con valores que cambian, ya en el hook useTheme estableciste una constante, utilizala siempre.

  1. Cuando cambia de tema se ve un pequeño bub en el boton de switch theme ya que es ghost y cambia los colores de fondo, eso creo que se puede arreglar con una transision de colores.

Sugerencia:
las constantes DARK_MODE y LIGHT_MODE simplemente se pueden abstraer a un enum

export enum ThemeMode {
Dark = "dark",
Light = "light"
}

Te sera mucho mas facil y es mas optimo que constantes sueltas creo.

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