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

New Button by Figma #167

Merged
merged 8 commits into from
Dec 14, 2023
Merged

New Button by Figma #167

merged 8 commits into from
Dec 14, 2023

Conversation

mdlucas
Copy link
Collaborator

@mdlucas mdlucas commented Dec 12, 2023

O que foi feito? 📝

Implementação do botão de acordo com o Figma usado pelos Designers
*Removi algumas props que não existem no figma como por exemplo: rounded, hasBorder.

Screenshots ou GIFs 📸

Captura de Tela 2023-12-12 às 17 34 52
Captura de Tela 2023-12-12 às 17 35 06
Captura de Tela 2023-12-12 às 17 35 21
Captura de Tela 2023-12-12 às 17 35 36
Captura de Tela 2023-12-12 às 17 35 48
Captura de Tela 2023-12-12 às 17 35 56
Captura de Tela 2023-12-12 às 17 46 10
Captura de Tela 2023-12-12 às 17 46 13
Captura de Tela 2023-12-12 às 17 46 21
Captura de Tela 2023-12-12 às 17 46 23

Tipo de mudança 🏗

  • Nova feature (mudança non-breaking que adiciona uma funcionalidade)
  • Bug fix (mudança non-breaking que conserta um problema)
  • Refactor (mudança non-breaking que melhora o código)
  • Chore (nenhuma das anteriores, como upgrade de libs)
  • Breaking change 🚨

Checklist 🧐

  • Testado no Yalc
  • Testado no Chrome
  • Testado no Safari

src/components/Button/styles.tsx Show resolved Hide resolved
src/components/Button/styles.tsx Show resolved Hide resolved
src/components/Button/styles.tsx Show resolved Hide resolved
src/components/Button/styles.tsx Outdated Show resolved Hide resolved
src/components/Button/styles.tsx Outdated Show resolved Hide resolved
src/components/Button/styles.tsx Show resolved Hide resolved
src/components/Button/styles.tsx Outdated Show resolved Hide resolved
| 'danger'
| 'success'
| 'warning'
| 'info';
Copy link
Collaborator

Choose a reason for hiding this comment

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

essas variants e colors deveriam estar vindo do theme-toolkit 🤔

Copy link
Collaborator Author

@mdlucas mdlucas Dec 12, 2023

Choose a reason for hiding this comment

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

acho que não, as variants são do botão, manter na fluid acho que é a melhor opção até para manutenção
assim caso mude como foi o caso, não precisa gerar versão de outra lib para alterar o type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sei que é chato, mas entendo que é parte do necessário isso estar vindo do Toolkit para não ficar desalinhado com a estrutura em si do Fluid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

é que eu acho estranho ter um componente na lib que é tipado por outra lib

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ammichael acha melhor deixar esse type no theme-toolkit mesmo ?

added padding from theme
@mdlucas
Copy link
Collaborator Author

mdlucas commented Dec 12, 2023

@ammichael esse é o figma do Torra, o pessoal de design ta usando esse tipo de nomenclatura para os botões, por isso alterei as varitants, agora temos filled, ghost, tint e outline, cada uma mudando a cor(primary, danger e etc...) de acordo com o que é passado
Captura de Tela 2023-12-12 às 20 55 07

Copy link

@fabinppkbuilders fabinppkbuilders left a comment

Choose a reason for hiding this comment

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

Mesmo usando yalc eu não consegui utilizar o template-react com esse botão.

Tive que forçadamente instalar a "@radix-ui/react-icons" no template e mesmo assim as variants não funcionaram corretamente.

Ex do botão "ghost":
Captura de Tela 2023-12-13 às 09 30 10

Como vc usou o yalc pra testar?

@mdlucas
Copy link
Collaborator Author

mdlucas commented Dec 13, 2023

Mesmo usando yalc eu não consegui utilizar o template-react com esse botão.

Tive que forçadamente instalar a "@radix-ui/react-icons" no template e mesmo assim as variants não funcionaram corretamente.

Ex do botão "ghost": Captura de Tela 2023-12-13 às 09 30 10

Como vc usou o yalc pra testar?

tem mudanças no template tambem, mas so posso abrir o PR depois de gerar a versão aqui
por algum motivo o template estava editando o componente inteiro, o que nao faz sentido, removi as alterações de la pra centralizar tudo na lib
@fabinppkbuilders

Copy link

@fabinppkbuilders fabinppkbuilders left a comment

Choose a reason for hiding this comment

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

A falta das props removidas (rounded, hasBorder..) não poderá fazer outros projetos quebrarem? (breaking change)
Assim como a remoção da variant "primary" por exemplo.

Falando por mim, fica bem complicado aprovar algo que eu não consegui validar. Se o PR do template fosse aberto facilitaria mais as coisas. Seria possível?


export const Touchable = styled(TouchableComponent)<ButtonWrapperProps>`
${getStylesButton}
${({ size }) => !!size && sizeButton[size]}
width: ${({ fullWidth }) => (!!fullWidth ? '100%' : undefined)};

Choose a reason for hiding this comment

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

essa linha pode virar width: undefined;, isso pode ser um problema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

acredito que ele fique vazio e nao seta a propriedade, mas vou testar

display: flex;
align-items: center;
gap: ${spacingMd}px;
gap: 8px;

Choose a reason for hiding this comment

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

Esse tamanho fixo não poderá gerar problemas depois?

Copy link
Collaborator Author

@mdlucas mdlucas Dec 13, 2023

Choose a reason for hiding this comment

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

não, porque pode ser alterado via css caso mude o projeto, e nao temos um token especifico no tema para isso

variant={typographyVariant}
$buttonVariant={variant}
>
<TextButton className="text-button" variant={typographyVariant}>

Choose a reason for hiding this comment

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

Por que usar styled-component com className?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

acho que são coisas distintas, uma não sobrescreve a outra.
usei a classe para ficar facil alterar o estilo do texto do botao com css quando necessário

@mdlucas
Copy link
Collaborator Author

mdlucas commented Dec 13, 2023

A falta das props removidas (rounded, hasBorder..) não poderá fazer outros projetos quebrarem? (breaking change) Assim como a remoção da variant "primary" por exemplo.

Falando por mim, fica bem complicado aprovar algo que eu não consegui validar. Se o PR do template fosse aberto facilitaria mais as coisas. Seria possível?

@fabinppkbuilders
sim, concordo que é break change, vou abrir o PR no template

@luancurti luancurti dismissed ammichael’s stale review December 14, 2023 17:49

Comentários resolvidos

@mdlucas mdlucas merged commit 52fa2f5 into master Dec 14, 2023
2 checks passed
@mdlucas mdlucas deleted the feat/NewButton branch December 14, 2023 17:49
@fabinppkbuilders
Copy link

@mdlucas
Mesmo com a 1.2.0 da fluid-react e com a 0.3.0 da theme-toolkit eu continuei obtendo problemas de tipagem no template-react.

Será que estou fazendo algo errado pra testar ou é preciso algo mais para que essas alterações sejam validadas por completo?

@luancurti como vc validou?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants