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: 상품 상세 페이지 개편 #75

Merged
merged 28 commits into from
Apr 14, 2024
Merged

feat: 상품 상세 페이지 개편 #75

merged 28 commits into from
Apr 14, 2024

Conversation

Leejin-Yang
Copy link
Contributor

Issue

✨ 구현한 기능

  • 상품 상세 아이템 컴포넌트
  • 리뷰 컴포넌트
2024-04-06.14.31.36.mov

📢 논의하고 싶은 내용

우선 현재 작업한 내용해서 pr 올립니다.

  • 꿀조합 섹션은 해온의 컴포넌트 가져와서 쓰려합니다. 정렬 버튼, 스크롤 버튼도...
  • 레이아웃에서 상하단 바가 없는 minimal layout을 없애고 각 페이지에서 스타일을 주어야 하는걸로 보이네요. 지금처럼 하면 메인 안에 헤더가 있는 구조로 되어서 수정이 필요해 보입니다. 우선 고치지 않고 상세 페이지에서 main을 넣어서 했습니다. 의견 주시면 수정할게요.!
  • 인디케이터 영역을 따로 만들어서 레이아웃에 넣으면 좋아보여요. 이 부분도 의견 주세요.
  • 띄엄띄엄 개발해서 잘 기억이 나지 않습니다.. 리뷰 작성은 다음 이슈에서 진행하겠습니다.

🎸 기타

  • 특이 사항이 있으면 작성합니다.

⏰ 일정

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

Copy link

github-actions bot commented Apr 6, 2024

🔗 배포된 storybook 주소: https://65f015a4aed45406385006ee-kvjozjhmlb.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.

수고했습니다 황펭
레시피랑 다 완성하고 다시 요청 주세요!
빠르게 훑어봤을 땐 별 문제 없어보입니다~

레이아웃에서 상하단 바가 없는 minimal layout을 없애고 각 페이지에서 스타일을 주어야 하는걸로 보이네요. 지금처럼 하면 메인 안에 헤더가 있는 구조로 되어서 수정이 필요해 보입니다. 우선 고치지 않고 상세 페이지에서 main을 넣어서 했습니다. 의견 주시면 수정할게요.!

궁금한게 main 내부에 헤더가 들어가면 안 되는건가요???
그러면 다른 상하단 바가 없는 페이지는 다 layout 사용 없이 개별로 줘야하는 것...??

인디케이터 영역을 따로 만들어서 레이아웃에 넣으면 좋아보여요. 이 부분도 의견 주세요.

인디케이터 영역이 어딜 말하는걸까요?

onClick={debouncedToggleFavorite}
aria-label={`좋아요 ${currentFavoriteCount}개`}
>
<SvgIcon variant={isFavorite ? 'favoriteFilled' : 'favorite'} fill={isFavorite ? 'red' : theme.colors.gray4} />
<Text as="span" weight="bold">
<SvgIcon variant="favorite2" width={16} fill={isFavorite ? vars.colors.error : vars.colors.icon.light} />
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 error말고 색상 추가하는거 어때요?
순간 error라길래 혼동이 왔슴니다...

@Leejin-Yang
Copy link
Contributor Author

@hae-on

인디케이터 영역이 어딜 말하는걸까요?

스크린샷 2024-04-07 19 08 22

이겁니다.!

궁금한게 main 내부에 헤더가 들어가면 안 되는건가요???
그러면 다른 상하단 바가 없는 페이지는 다 layout 사용 없이 개별로 줘야하는 것...??

header, main, footer 가 같은 라인으로 가려고요. 펀잇 기본 헤더가 없는 애들은 각자의 헤더가 있고, 또 하단에 고정되는(등록 버튼 등)도 각자 따로 있으니 레이아웃 잡지 않고 하는건 어떤가요?

Copy link
Member

@xodms0309 xodms0309 left a comment

Choose a reason for hiding this comment

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

황펭 고생했어요~~~ 그 Text 컴포넌트로 쓸 수 있는 것들만 수정해주시면 좋을 것 같아요.
현재 레이아웃이 복잡해서 뭔가 공통 레이아웃으로 빼기 어려울 것 같아 저도 페이지별로 따로 레이아웃 잡는게 나을 것 같다고 봅니다..!

@@ -23,7 +23,7 @@ const SortButton = ({ option, onClick }: SortButtonProps) => {

export default SortButton;

const SortButtonContainer = styled(Button)`
const SortButtonContainer = styled.button`
Copy link
Member

Choose a reason for hiding this comment

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

이 파일은 마이그레이션 안하셨나용?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해온의 꿀조합에서 가져오려고 합니다.!

Comment on lines 31 to 48
export const categoryName = style({
fontSize: '1.4rem',
fontWeight: 500,
color: '#808080',
lineHeight: 1.4,
});

export const productName = style({
fontSize: '1.6rem',
fontWeight: 600,
lineHeight: 1.4,
});

export const productPrice = style({
fontSize: '2.2rem',
fontWeight: 600,
lineHeight: 1.4,
});
Copy link
Member

Choose a reason for hiding this comment

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

Text 컴포넌트 사용하면 될 것 같아요! 파일 색상들도 vars에서 가져와주세요~!~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 color, size, weight 다 있군요 화긴

Comment on lines 11 to 19
export const countBase = style({
fontSize: '1.4rem',
fontWeight: 500,
});

export const count = styleVariants({
favorite: [countBase, { color: vars.colors.gray4 }],
default: [countBase, { color: vars.colors.gray3 }],
});
Copy link
Member

Choose a reason for hiding this comment

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

여기도 Text 컴포넌트 쓸 수 있을 것 같네용

Comment on lines 15 to 17
export const memberName = style({
fontWeight: 600,
});
Copy link
Member

Choose a reason for hiding this comment

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

여기도!

@Leejin-Yang
Copy link
Contributor Author

@xodms0309 @hae-on

레이아웃은 현재 pr 올라온 페이지 머지되면 제가 한번에 할게요

Copy link

github-actions bot commented Apr 9, 2024

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

Copy link

github-actions bot commented Apr 9, 2024

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

Copy link

github-actions bot commented Apr 9, 2024

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

@Leejin-Yang
Copy link
Contributor Author

@xodms0309 @hae-on

스크린샷 2024-04-09 22 53 45

상품 상세에 꿀조합 추가했습니다.
그 과정에서 RecipeItem 코드를 수정했어요

현재 Text 컴포넌트는 classname이 적용되지 않는데, 타미가 작업한거에 수정한게 있더라고요? 그거랑 합쳐지면 스타일 정상 적용될 듯 합니다.

[문제 및 질문]

  • 현재 detail/1 로 url이 변경되면서 상세에서 헤더의 뒤로가기 버튼을 누르면 /detail로 갑니다. 이 부분 코드를 수정해야해요. TopBar 작업하는거에서 뒤로가기 부분 수정이 필요해보입니다.
  • 상세에서 꿀조합 전체보기를 하면 해당 상품이 포함된 꿀조합 검색 페이지로 가는게 맞을까요? 의견주세요

@hae-on
Copy link
Contributor

hae-on commented Apr 10, 2024

현재 detail/1 로 url이 변경되면서 상세에서 헤더의 뒤로가기 버튼을 누르면 /detail로 갑니다. 이 부분 코드를 수정해야해요. TopBar 작업하는거에서 뒤로가기 부분 수정이 필요해보입니다.

그러면 detail/id 일때만 조건문 세워서 바꾸면 되려나용??

상세에서 꿀조합 전체보기를 하면 해당 상품이 포함된 꿀조합 검색 페이지로 가는게 맞을까요? 의견주세요

넵넵 상품이 포함된 꿀조합 검색 페이지가 유일하게 모아둔 페이지 같네여?
뭔가 전체 보기 버튼 따로 없이 보여줘도 좋을 거 같은데 너무 많아지면 필요하겠져...
지금은 몇개 제한으로 보이고 있죠??? 3개인가요???

@Leejin-Yang
Copy link
Contributor Author

그러면 detail/id 일때만 조건문 세워서 바꾸면 되려나용??

링크 컴포넌트에서 상대경로 따라가는거 말고 진짜 뒤로가기가 있는지 봐야겠어요

지금은 몇개 제한으로 보이고 있죠??? 3개인가요???

넵 피그마에서 검색과 함께 가기로 했슴다

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.

더이상 수정사항 없는거 맞나요??
approve 하겠슴다~~

Copy link
Member

@xodms0309 xodms0309 left a comment

Choose a reason for hiding this comment

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

황펭 리뷰 몇개만 더 확인해주세요..!

<ul className={tagList}>
{tags.map(({ id, name }) => (
<li key={id} className={tag}>
<span>{name}</span>
Copy link
Member

Choose a reason for hiding this comment

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

여기도 Text 컴포넌트로 변경 가능할 것 같아요
근데 Badge 컴포넌트를 활용해볼 수 없으려나

Copy link
Contributor Author

Choose a reason for hiding this comment

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

radius가 좀 다르더라고요..

Comment on lines 61 to 69
<ul className={tagList}>
{tags.map(({ id, name }) => (
<li key={id} className={tag}>
<Text as="span" color="info" size="caption2" weight="medium">
{name}
</Text>
</li>
))}
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

이거 TagList 컴포넌트 아닌가요?!

Comment on lines 42 to 44
export const productPrice = style({
fontSize: '2.2rem',
});
Copy link
Member

Choose a reason for hiding this comment

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

피그마 확인해보니까 display1이라는 이름으로 사이즈 추가됐네요!

padding: '0 10px',
textAlign: 'center',
borderRadius: 4,
backgroundColor: '#f2f2f2',
Copy link
Member

Choose a reason for hiding this comment

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

bg-category!

))}
{recipeToDisplay.length < recipes.length && (
<li className={moreItem}>
{/*링크는 상품이 포함된 꿀조합 검색결과로 가는 것이 맞을듯?*/}
Copy link
Member

Choose a reason for hiding this comment

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

요거는 제가 이슈 80번 하면서 수정해놓겠습니다

@Leejin-Yang
Copy link
Contributor Author

@xodms0309

리뷰 반영했습니다~

Copy link

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

Copy link

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

Copy link
Member

@xodms0309 xodms0309 left a comment

Choose a reason for hiding this comment

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

구웃 고생하셨으요~~

@Leejin-Yang Leejin-Yang merged commit 4196d6b into feat/v2 Apr 14, 2024
2 of 3 checks passed
@Leejin-Yang Leejin-Yang deleted the feat/issue-61 branch April 14, 2024 20:22
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