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(header): add a publish new content button in the header #1349

Merged
merged 5 commits into from May 5, 2023
Merged

Conversation

victorhcb
Copy link
Contributor

#1238

Vi essa issue aberta há um certo tempo e achei interessante tentar implementar algo nesse sentido.
Obs.: Ainda estou aprendendo como ajudar por aqui no projeto então peço desculpas se tiver feito algo errado.

@vercel
Copy link

vercel bot commented Mar 28, 2023

@victorhcb 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 Mar 28, 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 May 5, 2023 4:10am

devajmeireles
devajmeireles previously approved these changes Apr 24, 2023
@laviodias
Copy link

Oi, Victor.
Gostei da funcionalidade, então vou te sugerir de antemão que implemente uma versão responsiva. Da forma como está, o header quebra em telas menores. Também é preciso resolver o conflito existente entre as branches.
Também tenho um ponto que acho interessante de se considerar, mas não é uma sugestão, é apenas para você pensar sobre. Faz sentido manter a exibição do botão uma vez que o usuário já esteja na página de criar uma nova postagem?

Fico a disposição para discutir esse ponto, se quiser.

@aprendendofelipe
Copy link
Collaborator

Olá @victorhcb, obrigado pelo PR! 💪

Me perdoe a demora em responder 😅

Ficou meio grande esse botão, né? Não acha que ficaria melhor um botão pequeno no estilo do GitHub?

image

Talvez usar o IconButton 👍

Ótimos pontos @laviodias! 💪

Com relação a responsividade, com base nas conversas do #1279, eu acho que o botão vai precisar ficar oculto na versão mobile.

Caixetadev
Caixetadev previously approved these changes May 4, 2023
change the style of added button in header
this commit changes the style of new added button on header

#1238
@victorhcb victorhcb dismissed stale reviews from Caixetadev and devajmeireles via e964697 May 4, 2023 10:49
@victorhcb
Copy link
Contributor Author

Show pessoal! Fiz algumas correções de acordo com o que foi passado...

Ficou meio grande esse botão, né? Não acha que ficaria melhor um botão pequeno no estilo do GitHub?

image

Sobre isso, realmente acho que não caberia um botão com texto... fiz a alteração para o ícone simples, vejam o que acham...

image

Com relação a responsividade, com base nas conversas do #1279, eu acho que o botão vai precisar ficar oculto na versão mobile.

Em relação a responsividade, fiz alguns testes usando a dev tools aqui e não sei se a responsividade ficou prejudicada... acredito que o botão não tenha afetado tanto a responsividade. O único dispositivo no qual há um problema de responsividade pelo DevTools já apresenta problemas na versão atual do Header... será que não valeria a pena um PR corrigindo o Header para esse ponto específico?

Seguem algumas imagens:

Iphone 12 PRO
image

Galaxy S20
image

Oi, Victor. Gostei da funcionalidade, então vou te sugerir de antemão que implemente uma versão responsiva. Da forma como está, o header quebra em telas menores. Também é preciso resolver o conflito existente entre as branches. Também tenho um ponto que acho interessante de se considerar, mas não é uma sugestão, é apenas para você pensar sobre. Faz sentido manter a exibição do botão uma vez que o usuário já esteja na página de criar uma nova postagem?

Sobre o ponto de manter o botão mesmo na página de publicação, fiquei na dúvida se deveria mesmo alterar nesse PR já que o botão também permanece no ActionMenu já existente... o que vocês pensam em relação a isso?

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.

Show @victorhcb! 💪

Sobre remover o ícone quando estiver na página de publicação, eu não acho necessário. 👍

Sobre o restante, por favor, veja meus comentários no código 🤝

pages/interface/components/Header/index.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
adds a quick access icon button for the 'publish new content' page in the header of the desktop view

#1238
@aprendendofelipe aprendendofelipe merged commit f59583c into filipedeschamps:main May 5, 2023
4 checks passed
@aprendendofelipe
Copy link
Collaborator

Em produção! 🚀🚀🚀

@victorhcb, bem vindo à Turma de contribuidores do TabNews! 👏👏👏

@victorhcb
Copy link
Contributor Author

Aaaaah que massaaa!!!!
Estou muito feliz de começar a contribuir por aqui... e a ideia é tentar fazer cada vez mais contribuições pois agora estou tentando começar a pegar o jeito hahaha
Inclusive... me desculpe pelos probleminhas bestas de implementação... espero ter mais ajudado mais do que atrapalhado hahahaah Inclusive... Sinto que acabei fazendo uma bagunça nos commits iniciais... isso não seria prejudicial?

vlwww por toda a ajuda :D

@victorhcb victorhcb changed the title feat(add new content button): add new content button on header feat(header): add a publish new content button in the header May 5, 2023
@fabiosvm
Copy link

fabiosvm commented May 5, 2023

Bacana!

@aprendendofelipe
Copy link
Collaborator

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

6 participants