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

fix: tag default variant #1854

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

viniciuslagedo
Copy link
Contributor

Summary

Resolve #1740

Examples

<Tag>Label</Tag> // renders secondary gray label
<Tag variant="primary">Label</Tag> // renders primary gray

@viniciuslagedo viniciuslagedo requested a review from a team as a code owner August 22, 2024 13:33
Copy link

vercel bot commented Aug 22, 2024

Deployment failed with the following error:

You don't have permission to create a Preview Deployment for this project.

View Documentation: https://vercel.com/docs/accounts/team-members-and-roles

@lucasaarcoverde lucasaarcoverde changed the title Fix: tag default variant fix: tag default variant Aug 22, 2024
@viniciuslagedo
Copy link
Contributor Author

I believe it will cause a breaking change

The most used case of this component is with gray color and secondary variant so we changed it

BREAKING CHANGE: Tag component without variant will render with secondary prop, so all the
implementation now need to change to explicity set it to primary to stay in the same way

fix #1740
@lucasaarcoverde
Copy link
Contributor

I believe it will cause a breaking change

Sim, isso causa uma breaking change no comportamento do código e visual também! Nós entendemos que isso pode ocorrer mais vezes em outros lugares e é um tradeoff de uma breaking change majoritariamente visual vs uma melhoria na DX desse componente.

Nesse caso, conversei com @matheusps pra gente resolver esse problema e conseguir passar esse tipo de mudança. A ideia é tratar como um bug fix e criar alguma tag no JSDoc que indique essa mudança. Alguma anotation como @changed algo assim que simbolize isso na documentação. Ainda estou pensando melhor como fazer isso, você tem alguma sugestão?

Compartilhando aqui só pra dar uma resposta em relação ao PR, uma vez que a gente definir isso podemos fazer o merge e criar uma issue para a atualização dessa anotation na documentação 😄

@lucasaarcoverde
Copy link
Contributor

Outro ponto: a gente até então não tinha utilizado o Breaking Change: no commit pois a lib estava na 0.x e toda mudança era uma possível breaking change. O problema é que essa anotação faz a release automática gerar uma versão major caso não seja tratado corretamente, que é o caso agora. Caso esse PR seja mergeado será lançado a 2.x. Para evitar isso criei esse PR #1872. Falta testar ainda o comportamento, uma vez que estiver testado e mergeado ai esse fica livre

@viniciuslagedo
Copy link
Contributor Author

Outro ponto: a gente até então não tinha utilizado o Breaking Change: no commit pois a lib estava na 0.x e toda mudança era uma possível breaking change. O problema é que essa anotação faz a release automática gerar uma versão major caso não seja tratado corretamente, que é o caso agora. Caso esse PR seja mergeado será lançado a 2.x. Para evitar isso criei esse PR #1872. Falta testar ainda o comportamento, uma vez que estiver testado e mergeado ai esse fica livre

Faz muito sentido! Boa!

@viniciuslagedo
Copy link
Contributor Author

viniciuslagedo commented Aug 26, 2024

Sim, isso causa uma breaking change no comportamento do código e visual também! Nós entendemos que isso pode ocorrer mais vezes em outros lugares e é um tradeoff de uma breaking change majoritariamente visual vs uma melhoria na DX desse componente.

Nesse caso, conversei com @matheusps pra gente resolver esse problema e conseguir passar esse tipo de mudança. A ideia é tratar como um bug fix e criar alguma tag no JSDoc que indique essa mudança. Alguma anotation como @changed algo assim que simbolize isso na documentação. Ainda estou pensando melhor como fazer isso, você tem alguma sugestão?

Compartilhando aqui só pra dar uma resposta em relação ao PR, uma vez que a gente definir isso podemos fazer o merge e criar uma issue para a atualização dessa anotation na documentação 😄

Saquei, me parece ser um bom caminho de fato, estamos com uma mudança pequena mas ainda assim uma breaking change. Eu vi que tem aquelas annotations @since e @deprecated talvez seja um caminho.

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

Successfully merging this pull request may close these issues.

Change the default value of the prop variant in the Tag component
2 participants