Skip to content

[WRFE-11](feat): 검색 및 결과 페이지 퍼블리싱 #60

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

Open
wants to merge 72 commits into
base: dev
Choose a base branch
from

Conversation

Joy0w0
Copy link
Contributor

@Joy0w0 Joy0w0 commented Nov 13, 2024

📖 개요

💻 작업사항

  • 검색 및 결과 페이지 퍼블리싱

💡 작성한 이슈 외에 작업한 사항

  • BottomNavigation export 수정

✔️ check list

  • 작성한 이슈의 내용을 전부 적용했나요?
  • 리뷰어를 등록했나요?

@Joy0w0 Joy0w0 added 💻 FE 프론트엔드 관련 이슈 ✨ FEAT 새로운 기능 추가 labels Nov 13, 2024
@Joy0w0 Joy0w0 self-assigned this Nov 13, 2024
@Joy0w0 Joy0w0 changed the title [WRFE-11] 검색 및 결과 페이지 퍼블리싱 [WRFE-11](feat): 검색 및 결과 페이지 퍼블리싱 Nov 13, 2024
Copy link
Contributor

@eric-hjh eric-hjh left a comment

Choose a reason for hiding this comment

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

궁금한점 몇가지 남깁니다!

Comment on lines +39 to +47
<input
type='text'
placeholder='검색어를 입력해주세요.'
className={`w-full rounded-md border border-zinc-300 px-5 py-3 font-light text-zinc-700 placeholder:text-zinc-400 focus:outline-none ${
isFocused ? 'placeholder:opacity-0' : 'placeholder:opacity-100'
}`}
onFocus={() => setIsFocused(true)}
onBlur={() => setIsFocused(false)}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Input 공통 컴포넌트 사용하시면 더 편하실거같아요!
  • state와 onFocus, onBlur 대신 focus:placeholder:opacity-0
    을 사용해보시면 원하시는 동작을 더 간결하게 사용하실 수 있을거 같아요!
  • Search Icon까지 인풋값을 입력했을 때에 대한 케이스도 고려해주시면 더 좋을거 같아요!

</Header>
</div>

<section className='px-4'>
Copy link
Contributor

Choose a reason for hiding this comment

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

레이아웃을 위한거라면 div태그를 사용하는게 저는 더 좋아보이는데 section 태그를 사용하신 이유가 있을까요??

/>
</div>

<section className='mt-7 flex flex-col'>
Copy link
Contributor

@eric-hjh eric-hjh Nov 19, 2024

Choose a reason for hiding this comment

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

위 리뷰와 동일합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

수정 전 Header 코드 같은데, 없으면 conflict 뜨나요????

Comment on lines +22 to +35
<div className='sticky top-0 z-20 bg-white'>
<Header>
<Header.Left>
<div className='mt-6'>
<Header.BackButton onClick={router.back} />
</div>
</Header.Left>
<Header.Right>
<div className='flex w-full justify-end'>
<Icon name='shopping-box' showBadge badgeCount={2} />
</div>
</Header.Right>
</Header>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

results 페이지랑 공통되는 부분은 layout으로 빼도 좋을거 같습니다!

<Header>
<Header.Left>
<div className='mt-6'>
<Header.BackButton onClick={router.back} />
Copy link
Contributor

Choose a reason for hiding this comment

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

BackButton default 동작이 router.back이어서 없어도 될거 같습니다!

<div className='sticky top-0 z-20 bg-white'>
<Header>
<Header.Left>
<div className='mt-6'>
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 없어도 될거 같은데, 넣으신 이유가 있을까요?
없어도 될것같단 이유는 없어야 기존 레이아웃에 더 잘 맞는다 생각합니다!

</div>
</Header.Left>
<Header.Right>
<div className='flex w-full justify-end'>
Copy link
Contributor

@eric-hjh eric-hjh Nov 19, 2024

Choose a reason for hiding this comment

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

위 코멘트와 동일한 의견입니다!

Copy link
Contributor

@ww8007 ww8007 left a comment

Choose a reason for hiding this comment

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

간단한 코멘트 몇개 남겼습니다.
좀 이해관계를 쉽게 가져가길 원한다면 UI 컴포넌트를 분리해봐도 좋을 것 같아요!

Comment on lines +22 to +23
<div className='sticky top-0 z-20 bg-white'>
<Header>
Copy link
Contributor

Choose a reason for hiding this comment

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

헤더가 sticky 하게 붙길 원한다면 header에 className을 넘길 수 있도록 해봐도 괜찮을 것 같아요

Comment on lines +59 to +74
{keywords.length > 0 ? (
<div className='flex flex-wrap gap-2'>
{keywords.map((keyword, index) => (
<div
key={index}
className='inline-block rounded-full border-none bg-[#F9FAFB] px-3 py-2 text-sm text-gray-700'
>
# {keyword}
</div>
))}
</div>
) : (
<Typography className='font-light' size='h6' color='black'>
아직 추천 검색어가 없어요.
</Typography>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 보통 이렇게 줄이 길어질 때 &&로 엮는게 조건을 판단하기 좋은 것 같아서 많이 그렇게 사용하는데
이 부분은 어떠신가요?

{keywords.length > 0 && <div>}
{!keywords.length > 0 && 
 <Typography className='font-light' size='h6' color='black'>
                아직 추천 검색어가 없어요.
 </Typography>
}

}
};

export {BottomNavigation};
Copy link
Contributor

Choose a reason for hiding this comment

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

export const로 처리해도 좋을 것 같아요

Ajeong-Im and others added 30 commits November 27, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 FE 프론트엔드 관련 이슈 ✨ FEAT 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants