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(primer/react): update version #1578

Merged

Conversation

ErickCReis
Copy link
Contributor

Mudanças realizadas

Atualiza versão da lib @primer/react: 35.25.1-> 36.4.0

Durante a execução do pr (#1577) encontrei esse problema (primer/react#3741), nessa nova versão o warning não é mais exibido.

A versão 36 já foi lançada faz alguns meses, e nela tiveram alguns breaking changes que foram descritos aqui: primer/react#3685

Na prática apenas 1 breaking change afetou o projeto visualmente, a alteração das opções de variant no componente FormControl.Validation, onde a opção "warning" foi removida.

O componente PasswordInput input foi afetado:

Antes Depois
Captura de Tela 2023-12-20 às 01 37 43 Captura de Tela 2023-12-20 às 01 37 31

E a página de edição também:

Antes Depois
Captura de Tela 2023-12-20 às 01 54 44 Captura de Tela 2023-12-20 às 01 54 06

Tipo de mudança

  • Atualização de pacote

Checklist:

  • As modificações não geram novos logs de erro ou aviso (warning).
  • Eu adicionei testes que provam que a correção ou novo recurso funciona conforme esperado.
  • Tanto os novos testes quanto os antigos estão passando localmente.

Copy link

vercel bot commented Dec 20, 2023

@ErickCReis is attempting to deploy a commit to the TabNews Team on Vercel.

A member of the Team first needs to authorize it.

@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 20, 2023

Obrigado pelo PR e por seguir o template, Erick!

Não analisei o código, mas vendo seu PR, não me parece que o variant="error" seja uma boa ideia para esses dois cenários. Talvez faça sentido usar o FormControl.Caption para a senha:

Text Input com Caption, Label, Input, Validation, Required Indicator e Loading Indicator

Caption: Provides additional context about the field to help users fill in the correct data or explain how the data will be used. Caption text should be as short as possible. Caption text may be displayed at the same time as a validation message, or it may be hidden if it only provides redundant information.

Fonte.

E para o Email, um Flash de informação ou aviso: "Um email de confirmação foi enviado para ${email}. O email será atualizado apenas após a confirmação.".

Você acha que faz sentido? Pode testar se fica bom na prática?

Eu fui testar localmente para ver se ocorreria um conflito visual aparecendo o Flash do e-mail e o de confirmação de salvamento caso eu atualizasse a descrição, mas quando eu atualizo o e-mail, a descrição é ignorada 😅. Isso pode ser melhorado em outro PR, não analisei quão complexo será. Acredito que futuramente o Flash de sucesso deverá ser substituído pelo Toast, mas me parece que o Flash do e-mail continuará fazendo sentido.

Copy link

vercel bot commented Dec 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tabnews ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 2:05am

@aprendendofelipe
Copy link
Collaborator

@ErickCReis, muito obrigado pelo PR! 💪

Será que não é bom já atualizar o @primer/octicons-react? Se não me engano, é uma dependência do @primer/react.

E eu concordo com o @Rafatcb 👍


Eu fui testar localmente para ver se ocorreria um conflito visual aparecendo o Flash do e-mail e o de confirmação de salvamento caso eu atualizasse a descrição, mas quando eu atualizo o e-mail, a descrição é ignorada

@Rafatcb, quando você diz "a descrição é ignorada", quer dizer que a mensagem "Salvo com sucesso" não é exibida? Se você usou o setGlobalMessageObject, ele só vai mostrar o último valor setado mesmo.

No caso de alteração de email, a mesma mensagem já pode deixar claro que as alterações foram salvas, mas que o email precisa ser confirmado.

@aprendendofelipe
Copy link
Collaborator

Com a atualização do Primer, toda ajuda é necessária para testar (em homologação através do link abaixo) se não quebramos algo na interface visual 💪💪💪

https://tabnews-git-fork-erickcreis-feat-update-primer-react-tabnews.vercel.app/

@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 20, 2023

@Rafatcb, quando você diz "a descrição é ignorada", quer dizer que a mensagem "Salvo com sucesso" não é exibida? Se você usou o setGlobalMessageObject, ele só vai mostrar o último valor setado mesmo.

Não mostrou a mensagem, mas analisando melhor agora, entendi que o problema é outro. Ao dar F5 na tela de edição de perfil, a descrição fica vazia, mas ao navegar para ela por meio da UI, aparece. Além disso, o e-mail fica com o texto undefined na primeira renderização.

No caso de alteração de email, a mesma mensagem já pode deixar claro que as alterações foram salvas, mas que o email precisa ser confirmado.

Refazendo o teste, concordo com você que a mensagem seria melhor assim. Será preciso tomar cuidado com a mensagem para não dar a entender que o novo e-mail também foi salvo, pois ele será considerado apenas após a confirmação, certo?

@aprendendofelipe
Copy link
Collaborator

Ao dar F5 na tela de edição de perfil, a descrição fica vazia, mas ao navegar para ela por meio da UI, aparece. Além disso, o e-mail fica com o texto undefined na primeira renderização.

Peguei aqui, de fato temos esses bugs 😒

@ErickCReis, não precisa lidar com esses bugs (pelo menos não nesse PR), pois não está relacionado. 🤝

Será preciso tomar cuidado com a mensagem para não dar a entender que o novo e-mail também foi salvo, pois ele será considerado apenas após a confirmação, certo?

Isso mesmo! 👍

@Rafatcb Rafatcb added the pendente Aguardando ação do autor do Pull Request label Dec 20, 2023
@ErickCReis
Copy link
Contributor Author

@Rafatcb

Não analisei o código, mas vendo seu PR, não me parece que o variant="error" seja uma boa ideia para esses dois cenários. Talvez faça sentido usar o FormControl.Caption para a senha:

Concordo, vou fazer alguns testes com o FormControl.Caption aqui.

E para o Email, um Flash de informação ou aviso: "Um email de confirmação foi enviado para ${email}. O email será atualizado apenas após a confirmação.".

Você acha que faz sentido? Pode testar se fica bom na prática?

Sim, acho que faz mais sentido mesmo, vou verificar como fica.


@aprendendofelipe

Será que não é bom já atualizar o @primer/octicons-react? Se não me engano, é uma dependência do @primer/react.

Sim, não tinha notado isso, vou atualizar tbm. Dei uma olhada no changelog e não encontrei nada que possa afetar o projeto.

Com a atualização do Primer, toda ajuda é necessária para testar (em homologação através do link abaixo) se não quebramos algo na interface visual 💪💪💪

Com certeza, acabei não testando muito no meu ambiente local, nesse ambiente de homologação vai dar para testar mais cenários.

@ErickCReis, não precisa lidar com esses bugs (pelo menos não nesse PR), pois não está relacionado. 🤝

Ok, vou focar nesses pontos que o @Rafatcb comentou.

@ErickCReis
Copy link
Contributor Author

ErickCReis commented Dec 20, 2023

@Rafatcb

No input de senha utilizei o FormControl.Caption e acho que ficou bem melhor, o caption não aplica nenhum efeito na borda do input, mas mesmo assim gostei do resultado.
Você acha que precisa de mais destaque? Eu não apliquei nenhuma modificação no estilo padrão do Primer.

Captura de Tela 2023-12-20 às 19 32 15

Na tela de perfil utilizei o Flash e também achei que ficou melhor que antes.

Captura de Tela 2023-12-20 às 19 23 35

Vou continuar testando o ambiente de homologação, se tiver mais alguma sugestão é só avisar.

Copy link
Collaborator

@Rafatcb Rafatcb left a comment

Choose a reason for hiding this comment

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

Ficou muito legal, Erick 🥳. Não acho que precisamos mudar o estilo, do jeito que está ficou bom.

Deixei uma sugestão para melhorar a mensagem do Flash.

pages/perfil/index.public.js Outdated Show resolved Hide resolved
@Rafatcb Rafatcb removed the pendente Aguardando ação do autor do Pull Request label Dec 21, 2023
@aprendendofelipe aprendendofelipe merged commit 2e42e9e into filipedeschamps:main Dec 21, 2023
5 checks passed
@aprendendofelipe
Copy link
Collaborator

@ErickCReis, muito obrigado por mais essa contribuição! 💪💪💪

Já está rodando em produção 🚀🚀🚀

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

3 participants