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: 태그 검색 기능 구현 #64

Merged
merged 25 commits into from
Apr 11, 2024
Merged

feat: 태그 검색 기능 구현 #64

merged 25 commits into from
Apr 11, 2024

Conversation

xodms0309
Copy link
Member

@xodms0309 xodms0309 commented Mar 23, 2024

Issue

✨ 구현한 기능

일반 검색

스크린샷 2024-03-31 오후 10 56 56

태그 검색

스크린샷 2024-03-31 오후 10 57 19

검색 더보기 페이지

스크린샷 2024-03-31 오후 11 05 27

📢 논의하고 싶은 내용

  • 그 만들어놓은 Text 컴포넌트에서 스타일을 확장하고 싶을 때 어떻게 해야할지 몰라서 일단 className을 받게 했거든요?? 이거 어떻게 하면 좋을지도 의견 부탁드려요

🎸 기타

  • RecipeSearchResultList는 해온이 작업한 꿀조합 아이템 머지되면 반영해야합니다
  • 태그 검색이 들어가면서 코드가 매...~~~~~우 더러워졌습니다ㅜ ㅜ 양해 부탁
  • 제가 놓친 부분 있으면 꼭 알려주세요...

⏰ 일정

  • 추정 시간 : 1시간
  • 걸린 시간 : 4시간

@xodms0309 xodms0309 requested review from Leejin-Yang and hae-on March 23, 2024 14:54
@xodms0309 xodms0309 self-assigned this Mar 23, 2024
Copy link

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-zgjmihsqio.chromatic.com/

Copy link
Contributor

@hae-on hae-on left a comment

Choose a reason for hiding this comment

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

수고했습니다 타미~
저는 뱃지 영역 컴포넌트화 하는 거 괜춘!
코드만 봤을 때 뱃지 영역이 어디인지 한눈에 잘 안들어와서 만들면 괜찮을 듯??
그리고 ProductSearchResultList 이 컴포넌트는 스토리북이 없나요??
스토리북 배포 페이지에서 안보이는건지 제가 못 찾는건지 모르겠슴니당

@xodms0309
Copy link
Member Author

이거 일단 태그 검색 디자인이 아예 다르게 빠져가지고 페이지를 나누던가 해야될 것 같아요... 이 브랜치에서 작업하고 다시 리뷰 요청 드리겠슴다!

@xodms0309 xodms0309 marked this pull request as draft March 26, 2024 16:23
Copy link

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-ggsraspyox.chromatic.com/

Copy link

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-fhrtfqiuvp.chromatic.com/

@xodms0309 xodms0309 requested a review from hae-on March 31, 2024 21:06
@xodms0309 xodms0309 marked this pull request as ready for review March 31, 2024 21:06
Copy link
Contributor

@Leejin-Yang Leejin-Yang left a comment

Choose a reason for hiding this comment

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

와.. 조건 진짜 많네요 수고했어요 타미
태그 검색 결과 페이지랑 이름 검색 결과 페이지를 나누는건 어떤가요?
검색 추천, 자동완성 / 상품, 꿀조합 결과 / 태그 결과 / 상품 검색 더보기 / 꿀조합 검색 더보기 느낌으로
그리고 헤더쪽 스타일 변화가 있네요.!

@hae-on
Copy link
Contributor

hae-on commented Apr 6, 2024

타미! 제가 댓글단 이후 수정된 내용 pr에 적어두신건가용??? 그리고 스토리북에 추가된 애 검색 결과가 없다는 것만 보입니다~~ mock data 추가해주세욥!

@xodms0309
Copy link
Member Author

@Leejin-Yang
그 검색 결과 페이지를 나누자는게 검색 결과를 url 뒤에 query를 붙이지 말고 아예 새로운 페이지로 라우팅 되게 하자는 말씀이실까요?!

@Leejin-Yang
Copy link
Contributor

@xodms0309

스크린샷 2024-04-06 19 27 53

우선 궁금한게 이 두개의 경우에서 url이 현재 어떻게 될까요? /search?tag='맛있어요' , /search?query='미에로' 이런 형태일까요?
저의 의견은 /search/tags?query='맛있어요' , /search?query='미에로' 이렇게 나눠서 페이지 분리하는게 어떤지?
SearchPage에서 처리하는게 많아 보여서요

@xodms0309
Copy link
Member Author

xodms0309 commented Apr 7, 2024

@hae-on @Leejin-Yang

그 review2에 해당하는 아이콘 디자인이 피그마랑 다르길래 교체했어요. 근데 얘는 밖에서 fill 넣어줘도 안먹더라고요?? 근데 저 부분에서만 쓰이는 것 같아서 그냥.. 색 안넣고 쓰는걸로..

그리고 일단 그 헤더 디자인 변경된건 해온꺼 작업이랑 충돌될까봐 안건드렸는데.. 어떻게 할가요???

Copy link

github-actions bot commented Apr 7, 2024

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-jvujdqwbnw.chromatic.com/

@hae-on
Copy link
Contributor

hae-on commented Apr 7, 2024

@xodms0309

헤더는 그냥 수정 안하고 올리면 될 듯~~ 어차피 이거 머지되면 제가 헤더 작업하는 곳에서 끌어다써야해서 나중에 헤더 머지되고 제가 또 이슈 파서 헤더 다 갈아끼울게요!

Copy link
Contributor

@hae-on hae-on left a comment

Choose a reason for hiding this comment

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

타미 요것만 봐주세용~

...props
}: TextProps<T>) => {
const Component = as || 'p';

return (
<Component className={text({ color, size, weight })} {...props}>
<Component className={cx(text({ color, size, weight }), className)} {...props}>
Copy link
Contributor

Choose a reason for hiding this comment

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

전 요 방법 좋아보입니다~

Comment on lines 25 to 30
<ul className={container}>
{products.map(({ id, categoryType, image, name, price, averageRating }) => (
<li key={id}>
<Link to={`${PATH.PRODUCT_LIST}/${categoryType}/${id}`}>
<ProductOverviewItem image={image} name={name} price={price} rate={averageRating} />
</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

요부분 제 레시피 PR에 있는 ProductOverviewList 컴포넌트랑 똑같네요! 제 PR 머지되면 그거 고대로 넣으면 될 듯!

Copy link
Member Author

Choose a reason for hiding this comment

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

옹 좋아요

Comment on lines 35 to 43
<ul className={container}>
{products.map(({ id, categoryType, image, name, price, averageRating }) => (
{productToDisplay.map(({ id, categoryType, image, name, price, averageRating }) => (
<li key={id}>
<Link to={`${PATH.PRODUCT_LIST}/${categoryType}/${id}`}>
<ProductOverviewItem image={image} name={name} price={price} rate={averageRating} />
</Link>
</li>
))}
</ul>
Copy link
Contributor

@hae-on hae-on Apr 7, 2024

Choose a reason for hiding this comment

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

이 컴포넌트의 이름을 보고 떠오른거는 전체 상품 검색한 리스트를 다 뽑아오는건가 생각이 드는데
실제로는 전체중에 2개, 더보기 버튼 이렇게 나오더라구요.

그래서 아예 이 컴포넌트 이름을 변경했으면 좋겠어요!

밑에도 적어놨는데 이거 제 레시피 PR에 있는 ProductOverviewList 재사용하면 좋을 거 같은데,
중간에 자르는게 들어가있는게 걸리네요...

Copy link
Member Author

Choose a reason for hiding this comment

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

그냥 products에 productsToDisplay 넘겨주는거 어때여?! 컴포넌트 이름은 변경할게요!

<ProductOverviewItem image={image} name={name} price={price} rate={averageRating} />
</Link>
<div style={{ height: '20px' }} />
<hr style={{ border: '0.5px solid #e6e6e6' }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

줄이 있냐 없냐가 조금 다르긴한데, 이거는 조건써서 만들거나 하면 될 듯?
아니면 아예 상품 검색 리스트 컴포넌트를 따로 만드는 방법도 있고!

Copy link
Member Author

Choose a reason for hiding this comment

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

🤯
조건때문에 머리 터질거 같아요

Copy link

github-actions bot commented Apr 8, 2024

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-tsazqtybwa.chromatic.com/

@xodms0309
Copy link
Member Author

xodms0309 commented Apr 8, 2024

@Leejin-Yang @hae-on

  • 피그마에 올라온 버튼 디자인 변경 반영
  • ProductResultList -> ProductResultPreviewList로 교체
  • 해온이 만든 ProductOverviewList로 교체

그리고 지금 검색에서 꿀조합 관련도 디자인이 변경된 것 같은데 새로운 이슈파서 진행하겠습니다~!

Copy link

github-actions bot commented Apr 8, 2024

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-mwrirxsxzx.chromatic.com/

Copy link
Contributor

@hae-on hae-on 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 +22 to +27
{isSearchPage && (
<>
<div style={{ height: '20px' }} />
<hr style={{ border: '0.5px solid #e6e6e6' }} />
</>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

짱 👍

<ul className={container}>
{products.map((product) => (
<li key={product.id}>
<Link to={`${PATH.PRODUCT_LIST}/${product.categoryType}/${product.id}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

타미 이거 혹시 상품 상세로 넘어가는건가요?
만약 맞다면 ${product.categoryType} 이부분을 detail로 수정해야함니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

아 맞다!!!! 이거 고친다해놓고 깜빡했네요

@xodms0309
Copy link
Member Author

@hae-on @Leejin-Yang 마지막 검토 부탁드립니당.

Copy link

github-actions bot commented Apr 9, 2024

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-ziupfkznhj.chromatic.com/

Copy link
Contributor

@Leejin-Yang Leejin-Yang left a comment

Choose a reason for hiding this comment

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

수고했어요~
큰건 아니고 의견 하나 달았는데 이거만 확인해주세요.!

queries: `?query=${query}&lastProductId=${pageParam}`,
});
const data: ProductSearchResultResponse = await response.json();

return data;
};

const useInfiniteProductSearchResultsQuery = (query: string) => {
const useInfiniteProductSearchResultsQuery = (query: string, isTagSearch = false) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

true, false는 좀 추상적이여서 endpoint 자체를 인자로 받는건 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하고 머지하겠슴니당

Copy link

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-xcdovqgqdt.chromatic.com/

@xodms0309 xodms0309 merged commit ae36aa8 into feat/v2 Apr 11, 2024
2 of 3 checks passed
@xodms0309 xodms0309 deleted the feat/issue-60 branch April 11, 2024 14:28
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.

태그 검색 구현
3 participants