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

JUIP-148 Crear componente Button #35

Merged
merged 18 commits into from
Jun 6, 2024

Conversation

dam788
Copy link

@dam788 dam788 commented Apr 16, 2024

LINK DE TICKET:

https://janiscommerce.atlassian.net/browse/JUIP-143

DESCRIPCIÓN DEL REQUERIMIENTO:

Contexto
Actualmente tenemos un boton en la libreria que lo conocemos como BaseButton que hace todo.

Necesidad
Crear un nuevo componente de botón que pueda ser utilizado dentro de la librería de componentes existente. Este botón debe tener la capacidad de aceptar varias propiedades para personalizar su apariencia y comportamiento, incluyendo variantes de estilo, colores personalizados y la opción de incluir un icono.

¿Qué necesitamos lograr con este botón? Nuestro objetivo es desarrollar un botón versátil que pueda adaptarse a diferentes estilos de diseño y necesidades de nuestras aplicaciones. Queremos que sea una herramienta poderosa que nos ayude a mantener una apariencia coherente en todas nuestras aplicaciones, al tiempo que nos permite reutilizar fácilmente el código.
Características del botón:

Estilos adaptables:

El botón debe admitir dos estilos diferentes: "outlined" , "contained" y “text“. Esto nos permitirá usarlo en una variedad de contextos y respetar las especificaciones de diseño de cada aplicación.

Personalización de colores:

Utilizaremos la paleta de colores existente, que incluye colores primarios, secundarios, grises y de estado. Esto asegurará que el botón se integre perfectamente en nuestras aplicaciones y mantenga una apariencia coherente en todo momento.

Soporte para iconos:

El botón debe ser capaz de incluir un icono, ya sea antes o después del texto. La inclusión de iconos es opcional, pero necesaria en ciertos casos.

Objetivos:

Facilitar el trabajo del equipo al proporcionar un botón que sea fácil de integrar y personalizar en nuestras aplicaciones.

Promover la reutilización de código y mantener una apariencia coherente en todas nuestras aplicaciones.

Brindar una oportunidad de aprendizaje y crecimiento para el desarrollador encargado de esta tarea.

Que vamos a conseguir con esto?
No vamos a usar mas el BaseButton exportandolo desde la libreria, sino que el componente Button lo usara internamente como base para extender y usaremos en las distintas apps el componente Button. (Tener en cuenta comentario mas abajo que el cambio tiene que ser retro)

Contar con un boton nuevo que permita mediante props mostrarlo segun el diseño planteado en los rediseños.

DESCRIPCIÓN DE LA SOLUCIÓN:

  • se modificó BaseButton para llevarlo a la minima expresión del cual podemos extender para crear nuevos desarrollo de botón según convenga.
  • Se creó el componente Button el cual posee las siguientes props:

2024-04-16_17-39

CÓMO SE PUEDE PROBAR?

Desde ui-native:

  • Copiamos esto en App.tsx:
import React from 'react';
import {View} from 'react-native';
import Button from './src/components/Button';

const App = () => (
	<View style={{padding: 20, flexDirection: 'row'}}>
		<Button
			value={null}
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'contained'}
			color={'primary'}
			// iconPosition={'right}
			disabled={false}
			// style={}
		/>
		<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'outlined'}
			color={'primary'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6, flex: 1}}
			iconStyle={{color: 'red', fontSize: 30}}
		/>
	</View>
);

export default App;
  • Ahora tiramos los comandos: npm start y npm run android

Desde storybook:

  • Tirar comando: npm start y npm run storybook:android

Desde una de la app:

  • Tirar comando yalc push && yalc publish en ui-native .
  • En la app que quieras probar: yalc add @janiscommerce/ui-native y npm i.

SCREENSHOTS:

Peek 2024-04-16 17-46

LINK PR QA:

DATOS EXTRA A TENER EN CUENTA:

Figma -> LINK

CHANGELOG:

Copy link
Contributor

@pablonortiz pablonortiz left a comment

Choose a reason for hiding this comment

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

Lo estuve probando en Delivery y lo vi bien, me parece perfecto cómo fuiste sacando la lógica en distintas utils y constantes, lo que dejo el componente bastante limpio y legible

Copy link
Contributor

@GonzaFran GonzaFran left a comment

Choose a reason for hiding this comment

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

@dam788 lo probé en WMS y lo vi muy bueno, lo único que agregaría sería que, en las props, te sugiera opciones para usar en las propiedades [variant, color, iconPosition, etc.], algo así:
Captura desde 2024-04-17 09-54-01

Fijate que pablo lo había armado para el componente avatar. Si es fácil de adaptar buenisimo, si es mucho laburo dejalo así, pero creo que facilita mucho su uso si te sugiere valores para las props.
Cualquier cosa avisame y lo vuelvo a ver.

src/components/Button/theme/configs/index.ts Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 17, 2024

Pull Request Test Coverage Report for Build 9403895608

Details

  • 65 of 65 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8191728363: 0.0%
Covered Lines: 506
Relevant Lines: 506

💛 - Coveralls

@dam788 dam788 requested a review from GonzaFran April 17, 2024 14:53
Copy link

@Maukr Maukr left a comment

Choose a reason for hiding this comment

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

Hola @dam788 3 cosas que noté haciendo pruebas:
1- El type secondary y type primary cambian el alto del botón, esto es esperado?
Prueba:

		<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'secondary'}
			variant={'outlined'}
			color={'primary'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>
		<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'outlined'}
			color={'primary'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>

image

2- La prop color solo funciona para la variante por defecto? Si quiero poner success con la variante "outlined" no estaría haciendo efecto, no cambia a verde la font y el ícono.

Prueba:

<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'outlined'}
			color={'success'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>

image

3- Esto es un detalle, por algún motivo en especial se definió que el "value" por defecto sea "Button"? No sería mejor que no aparezca nada así no tenemos que enviarlo como null?

@dam788
Copy link
Author

dam788 commented Apr 17, 2024

Hola @dam788 3 cosas que noté haciendo pruebas: 1- El type secondary y type primary cambian el alto del botón, esto es esperado? Prueba:

		<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'secondary'}
			variant={'outlined'}
			color={'primary'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>
		<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'outlined'}
			color={'primary'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>

image

2- La prop color solo funciona para la variante por defecto? Si quiero poner success con la variante "outlined" no estaría haciendo efecto, no cambia a verde la font y el ícono.

Prueba:

<Button
			value="Button with Box Icon"
			icon="box"
			isLoading={false}
			type={'main'}
			variant={'outlined'}
			color={'success'}
			iconPosition={'left'}
			disabled={false}
			style={{marginLeft: 6}}
		/>

image

3- Esto es un detalle, por algún motivo en especial se definió que el "value" por defecto sea "Button"? No sería mejor que no aparezca nada así no tenemos que enviarlo como null?

Mauro te respondo una por una:
1 - La diferencia entre un type main y secondary es:

  • la altura en el main es 50 y en el secondary 42.
  • cuando es variant outlined el texto en el main es black y en el secondary es el color seleccionado.
  • en algunos casos muestra sombra, eso lo habia agregado pero se lo quité porque al revisar el flujo en delivery no tiene por lo que se puede intuir que la sombra se la agregan aparte por estilos.

LINK

2 - En éste caso como muestra en el link del diseño de los botones, en main no es que no hace efecto sino que éste es asi...

3 - En realidad, mi idea era que si no tenia value o icon, no se mostrara nada pero ahi surgieron dos problemas:

  • Un error en los hooks cuando devuelve un null o un <></>.
  • En la tabla de props que dejo fer en el ticket, puso que value es requeriada.

Entonces para solucionar ambos errores, deje por defecto el value="Button", pero teniendo en cuenta que hay botones sin texto deje abierta la posibilidad de pasarle un null (para que no se muestre el texto y solo el icono) lo cual no se si esta bien del todo. Vos decime, si queres mandamos null, y comentamos con eslint-ignore las lineas donde me salen los errores en hooks.

Copy link
Contributor

@christian97dd christian97dd left a comment

Choose a reason for hiding this comment

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

Dejé unos comentarios, pero por el momento dejamos un poco pausada esta tarea para poder darle foco al flujo de movimientos de productos de wms.
Mientras tanto usemos el button que tenemos en cada app y para el flujo nuevo le agregamos los bordes redondeados

onPressOut = () => {},
...props
}) => {
const [isPressed, setIsPressed] = useState<Boolean>(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

ojo con tener un setState en un button, puede llegar a ser muy costoso a nivel de rerenders, tené en cuenta que estas haciendo 2 setState por cada vez que el usuario presiona un botón.
Entiendo que lo estas usando para captar cuando está presionado el botón.
Haciendo esto que dice este comentario, te ahorras el setState.
Fijate que incluso en cada app lo resolvemos de la misma manera en src/components/BaseButton/index.js línea 56

Copy link
Author

Choose a reason for hiding this comment

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

la idea original era esa misma, pero se complicó cuando quise extender los estilos de BaseButton a Button. Ahi dejó de funcionar y tuve que buscar una alternativa

@@ -0,0 +1,34 @@
import {alert, base, black, error, primary, success, warning} from '../../../theme/palette';

export const themeColors = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fijate que te estas creando una nueva paleta de colores atado unicamente al botón, no va a resultar escalable si el día de mañana lo llegasemos a necesitar en cualquier otro componente que siga el mismo lineamiento.
Según los criterios en el ticket, se definió usar el mismo theme que hoy tenemos, agregando las keys necesarias, por ejemplo:

const theme = {
  primary: {
    light: #fff
    main: #234
    dark: #sddfsdf
    text: #fff
  }
}

}, [onPressOut]);

const LoadingCompontent = (
<Loading isLoading={isLoading} color={buttonStyle.loadingColor} size={24} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Fijate que cuando tenemos el isLoading, va cambiando el color del spinner según la prop color:
image
Siempre tenemos que mantenes el azul de janis

Copy link
Author

Choose a reason for hiding this comment

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

Es verdad, crei que tenia que tomar el color seleccionado. Buen punto

@dam788 dam788 requested a review from christian97dd May 31, 2024 17:10
Copy link

@WilliamSaya-lvl30 WilliamSaya-lvl30 left a comment

Choose a reason for hiding this comment

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

Lo veo bien

Copy link
Contributor

@christian97dd christian97dd left a comment

Choose a reason for hiding this comment

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

Lo ví mucho mejor, lo único que me hace ruido es cuando se le pasa el isLoading en true, a penas se vé:
image

Convendría deshabilitarlo y que quede siempre el loading con el color de janis:
image

@christian97dd christian97dd merged commit e529061 into master Jun 6, 2024
2 checks passed
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.

None yet

7 participants