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

refactor(icon imports): change icon imports to TabNewsUI/icons #1489

Merged

Conversation

ValbertMartins
Copy link
Contributor

• Esse pr refatora a importação dos icones para um só lugar, /TabNewsUI/icons.

• a subpasta icons é para não misturar a importação de componentes e icones tudo dentro de TabNewsUI

@vercel
Copy link

vercel bot commented Jul 27, 2023

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

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jul 27, 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 Jul 27, 2023 9:36pm

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

@ValbertMartins, obrigado pelo PR! 💪

Faltou exportar o CommentIcon.

Também fiz algumas sugestões no código 👍

pages/[username]/index.public.js Outdated Show resolved Hide resolved
pages/[username]/index.public.js Outdated Show resolved Hide resolved
pages/interface/components/Header/index.js Outdated Show resolved Hide resolved
pages/interface/components/Header/index.js Outdated Show resolved Hide resolved
@brunofamiliar
Copy link
Contributor

brunofamiliar commented Jul 27, 2023

Sei que no momento não é uma dor. Mas já tive problemas quando um projeto começa a escalar em quantidade de features onde é necessário uma quantidade maior de ícones. Utilizar lib's assim pode onerar no sentido de que, para cada novo ícone teremos que entrar em "@/TabNewsUI/icons", adicionar esse novo ícone, para só então conseguir utilizar em outra parte do sistema. Não seria viável utilizar soluções como por exemplo o iconify? Ele consegue centralizar várias soluções das principais lib's de icones existentes (assim como o react-icons faz). O diferencial é que é importado apenas um único componente (<Icon />), passando como parâmetro o icon desejado.

@aprendendofelipe
Copy link
Collaborator

aprendendofelipe commented Jul 30, 2023

Utilizar lib's assim pode onerar no sentido de que, para cada novo ícone teremos que entrar em "@/TabNewsUI/icons", adicionar esse novo ícone, para só então conseguir utilizar em outra parte do sistema.

Não sei se entendi qual é o ponto que você quiz dizer com "lib's assim", mas esse PR eu não enxergo como algo que onera, mas sim como algo que organiza e facilita manutenção, já que fica muito mais fácil substituir qualquer biblioteca de ícones caso seja necessário. Também facilita para quem quiser adicionar ícones em qualquer ponto do sistema, pois fica fácil saber o que já existe ou não no projeto.

Não seria viável utilizar soluções como por exemplo o iconify? Ele consegue centralizar várias soluções das principais lib's de icones existentes (assim como o react-icons faz). O diferencial é que é importado apenas um único componente (<Icon />), passando como parâmetro o icon desejado.

Não sei se existiria alguma vantagem em usar essa lib especificamente, mas, se for o caso, eu tentaria usá-la de maneira que fosse fácil de substituí-la a qualquer momento, ou seja, importaria a biblioteca em um único arquivo e usaria no restante do sistema através desse ponto centralizado, assim como está sendo feito nesse PR.

@aprendendofelipe aprendendofelipe merged commit 49aadd3 into filipedeschamps:main Jul 30, 2023
5 checks passed
@aprendendofelipe
Copy link
Collaborator

Em produção 🚀🚀🚀

@aprendendofelipe
Copy link
Collaborator

@ValbertMartins, você foi citado no post comemorativo:

https://www.tabnews.com.br/FelipeBarso/parabens-tabnews-aniversario-de-lancamento-e-mais-melhorias 🎉

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