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

Issue 7 - Navbar #31

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Issue 7 - Navbar #31

wants to merge 15 commits into from

Conversation

muritadb
Copy link

Closes #7

Feature
Criar Navbar do Site Petdex

Solution

image

image

image

Checklist

  • Issue linked
  • Build working correctly

Additional info

Comment on lines 25 to 33
<div
className={`${!isOpen ? '' : 'left-[-400px]'} absolute flex flex-col left-0 top-[8rem] lg:relative lg:left-0 lg:top-0 `}
>
Copy link
Member

@k1nha k1nha Aug 23, 2024

Choose a reason for hiding this comment

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

acho que essa abordagem não é a das melhores usando tailwind, bom utilizar clsx/tailwindmerge - tem um link para referência e ele está no repo

além do position mudar de acordo com o tamanho da tela, não me parece certo

Copy link
Author

Choose a reason for hiding this comment

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

Vou le esse artigo, valeu

Copy link
Member

Choose a reason for hiding this comment

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

é bom criar tipagem para todas as props de componente, é valido dar uma olhada no seu lint

Copy link
Member

@k1nha k1nha Aug 23, 2024

Choose a reason for hiding this comment

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

outro ponto valido, será que vale a pena colocar a responsabilidade de renderizacao pra dentro do componente?

Copy link
Author

Choose a reason for hiding this comment

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

" ...responsabilidade de renderizacao pra dentro do componente", acha melhor fragmentar em outro component ?
era uma abordagem que ia fazer mas pensei que colocar a renderizacao dentro dele seria melhor, pelo contexto e tals, mas pode ser, eu fragmento ele em 2 e uso a logica de renderizacao no nav, por exemplo:

{ !isSerarch 
    ?  <Logo />
    :  <LogoWithoutText /> 
}

Copy link
Member

Choose a reason for hiding this comment

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

essa logica deveria estar no componente pai, que é responsavel pela renderizacao dele

Copy link
Member

Choose a reason for hiding this comment

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

será que vale a pena colocar a responsabilidade de renderizacao pra dentro do componente?

Copy link
Contributor

@Frompaje Frompaje left a comment

Choose a reason for hiding this comment

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

Muito bom ver você codando em tudo, da uma olhada nos coments, e padroniza os arquivos(components) para PascalCase

components/petdex/Navbar/assets/Menu.tsx Outdated Show resolved Hide resolved
components/petdex/Navbar/assets/Logo-mobile.tsx Outdated Show resolved Hide resolved
@muritadb muritadb requested a review from Frompaje August 30, 2024 21:01
@juliaam
Copy link

juliaam commented Aug 30, 2024

atualiza a branch!

Copy link
Member

@Luiginicoletti Luiginicoletti left a comment

Choose a reason for hiding this comment

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

ci/cd bb

Copy link
Member

@PiluVitu PiluVitu left a comment

Choose a reason for hiding this comment

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

LGTM

@PiluVitu PiluVitu self-requested a review September 11, 2024 14:21
@PiluVitu PiluVitu marked this pull request as draft September 12, 2024 20:03
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.

navbar
6 participants