-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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í:
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.
Pull Request Test Coverage Report for Build 9403895608Details
💛 - Coveralls |
There was a problem hiding this 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}}
/>
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}}
/>
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:
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:
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. |
There was a problem hiding this 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
src/components/Button/index.tsx
Outdated
onPressOut = () => {}, | ||
...props | ||
}) => { | ||
const [isPressed, setIsPressed] = useState<Boolean>(false); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/components/Button/theme/index.ts
Outdated
@@ -0,0 +1,34 @@ | |||
import {alert, base, black, error, primary, success, warning} from '../../../theme/palette'; | |||
|
|||
export const themeColors = { |
There was a problem hiding this comment.
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
}
}
src/components/Button/index.tsx
Outdated
}, [onPressOut]); | ||
|
||
const LoadingCompontent = ( | ||
<Loading isLoading={isLoading} color={buttonStyle.loadingColor} size={24} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lo veo bien
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
CÓMO SE PUEDE PROBAR?
Desde ui-native:
npm start
ynpm run android
Desde storybook:
npm start
ynpm run storybook:android
Desde una de la app:
yalc push && yalc publish
en ui-native .@janiscommerce/ui-native
ynpm i
.SCREENSHOTS:
LINK PR QA:
DATOS EXTRA A TENER EN CUENTA:
Figma -> LINK
CHANGELOG: