Skip to content

Conversation

@melipefelgaco
Copy link
Contributor

@melipefelgaco melipefelgaco commented Sep 1, 2021

Descrição

  • Ao inves de utilizarmos um arquivo unico icons.tsx com todos os icones, cada um foi colocado em /packges/assets/icons em seu respectivo arquivo
  • Resolve a task

Changes

  • Cada icone foi colocado na nova pasta, importando somente o que eh utilizado nele
  • Os imports nao acontecem automaticamente entao estes foram corrigidos manualmente. Felizmente neste estagio do projeto ainda temos pouquissimos imports de icons entao foi bem tranquilo
  • Antigo arquivo icons.tsx deletado

Notes

  • Com o crescimento do projeto essa refatoracao nos icones eh bem-vinda e facilita bastante a legibilidade e manutencao do nosso codigo. Acho que ter feito isso agora que temos poucos icons foi melhor do que se tivessemos esperado o projeto crescer mais tambem. Isso nos lembra a pensar na oportunidade de refatorar outros codigos agora, caso os vejamos, para evitar dor de cabeca no futuro.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021

Visit the preview URL for this PR (updated for commit 16ed886):

https://podcodar-webapp--pr29-enh-icons-refactorin-027a2n36.web.app

(expires Wed, 08 Sep 2021 15:14:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Contributor

@marco-souza marco-souza left a comment

Choose a reason for hiding this comment

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

Mano, ta de parabéns! 👏👏👏
História de commit ta bem feitinha, bem pensada, gostei muito! Isso que é evolução! 🚀

Só deixei um comentário para criar um index.ts file que vai centralizar o export de icons. (@frattezi lemme know your thoughts on this.)

Links:

@melipefelgaco
Copy link
Contributor Author

melipefelgaco commented Sep 1, 2021

Mano, ta de parabéns! 👏👏👏
História de commit ta bem feitinha, bem pensada, gostei muito! Isso que é evolução! 🚀

Só deixei um comentário para criar um index.ts file que vai centralizar o export de icons. (@frattezi lemme know your thoughts on this.)

Links:

Fiquei me perguntando se nao tinha um jeito mais limpo de fazer o import, talvez essa seja a solucao entao! Legal, to lendo os links aqui e faco o fix logo apos

edit: @marco-souza pelo que li esse arquivo eh chamado por norma de barrel. sera que nao fica mais claro se ele for chamado de Barrel.tsx ou barrel.tsx? Ah nvm, parece q se a gente chamar de index nao precisa espeficiar o nome do arquivo nos imports, so a pasta

edit2: correcoes feitas

@marco-souza
Copy link
Contributor

Se o nome mudar ele não funciona. Tem que ser index.tsx, ou index.js pra javascript

@melipefelgaco
Copy link
Contributor Author

melipefelgaco commented Sep 1, 2021

Se o nome mudar ele não funciona. Tem que ser index.tsx, ou index.js pra javascript

sim, sim. eu percebi enquanto fazia o comentario, ta ali no edit haha
sry

Copy link
Contributor

@marco-souza marco-souza left a comment

Choose a reason for hiding this comment

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

Não acho uma boa mover eles para a pasta de assets, pois eles são componentes.

A pasta assets é pensada como uma pasta de multimedia imagens, videos, musica, documentos, etc. Tudo que não for código.

Como os icones são código, acho que deveriam estar na pasta de components

@melipefelgaco
Copy link
Contributor Author

Não acho uma boa mover eles para a pasta de assets, pois eles são componentes.

A pasta assets é pensada como uma pasta de multimedia imagens, videos, musica, documentos, etc. Tudo que não for código.

Como os icones são código, acho que deveriam estar na pasta de components

ok, pra mim icone era asset. fixed

@marco-souza
Copy link
Contributor

Icones são assets, mas veja que o arquivo nõa chama alguma-coisa.svg, ele é um tsx, ou seja, TypeScript.

@marco-souza
Copy link
Contributor

A gente ta transformando um svg que seria uma imagem crua em um componente react.

@melipefelgaco
Copy link
Contributor Author

melipefelgaco commented Sep 1, 2021

Icones são assets, mas veja que o arquivo nõa chama alguma-coisa.svg, ele é um tsx, ou seja, TypeScript.

faz sentido sim, no fim ele eh codigo

edit: all fixed. meu vscode ficou doido e nao deu stage change num dos arquivos, por isso vai ter um num commit separado eu acho =(

@melipefelgaco
Copy link
Contributor Author

Fixed. Aproveitei pra deletar um import que nao estava sendo usado no 404.tsx

Copy link
Contributor

@marco-souza marco-souza left a comment

Choose a reason for hiding this comment

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

Good to go

Copy link
Contributor

@Joel-leal Joel-leal left a comment

Choose a reason for hiding this comment

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

Foda!

@melipefelgaco melipefelgaco merged commit 2c02528 into main Sep 1, 2021
@melipefelgaco melipefelgaco deleted the enh/icons-refactoring branch September 1, 2021 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants