Conversation
빌드 결과빌드 성공 🥳 |
빌드 결과빌드 성공 🥳 |
1 similar comment
빌드 결과빌드 성공 🥳 |
3d3e210 to
d2cd9af
Compare
빌드 결과빌드 성공 🥳 |
odukong
left a comment
There was a problem hiding this comment.
별 아이콘 간격의 경우, 현재처럼 음수 마진을 활용하는 방식이 현 디자인 요구사항에서는 가장 적절한 방식이라고 생각해요. 썸네일 이미지 역시 기획 상 별도의 클릭 이벤트가 없는 상태여서 우선은 지금처럼 호버하였을 경우 포인터가 커서로 변경되어 인터렉션이 가능한 요소임을 나타내는 정도가 좋은 것 같습니다 !
추가적으로 수정하면 좋을 부분들 코멘트로 달아두었으니 참고 부탁드려요!! 수고하셨습니다🙌🏻
| export const caption = style({ | ||
| "@layer": { | ||
| [components]: { | ||
| ...typographyVars.body3, | ||
| color: color.gray[100], | ||
| margin: 0, | ||
| marginBottom: "0.8rem", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // 실제 후기 한눈에 보기 | ||
| export const title = style({ | ||
| "@layer": { | ||
| [components]: { | ||
| ...typographyVars.heading2, | ||
| color: color.black[100], | ||
| margin: 0, | ||
| marginBottom: "1.6rem", | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
후기 요약 컴포넌트 내에서 텍스트 요소들이 많이 사용되는 것 같습니당
이렇게 텍스트 요소들이 많이 사용될 때 하나하나 스타일을 지정하여 사용하기 보다는
recipe를 활용해 공통적으로 사용되는 스타일을 base로 지정하고, 각 텍스트 용도(캡션, 타이틀)에 따라 달라지는 부분만 variants로 분리 해두면 컴포넌트 내부에서 불필요한 스타일 중복을 줄일 수 있어요!!
header 요소 내부에 존재하는 텍스트들은 공통적으로 textAlign: "center"라는 속성을 가지고 있기 때문에 이를 base로 묶고, 나머지는 용도에 따라 스타일을 지정하여, headerText라는 하나의 스타일을 공유해 사용할 수 있을 것 같아요!
export const headerText = recipe({
base: {
"@layer": {
[components]: {
textAlign: "center", // 공통 속성
},
},
},
variants: {
type: {
caption: {
...typographyVars.body3,
color: color.gray[100],
marginBottom: "0.8rem",
},
title: {
...typographyVars.heading2,
color: color.black[100],
marginBottom: "1.6rem",
},
},
},
});이 recipe를 활용한 방식은 여러 유사한 스타일을 지정할 때 유용하게 사용할 수 있으니 참고해주세요🥰
| [components]: { | ||
| ...typographyVars.display, | ||
| color: color.black[100], | ||
| margin: 0, |
There was a problem hiding this comment.
margin : 0 마진에 대한 값이 여러 스타일에 중복적으로 사용되고 있는데, 유의미한 의미를 가지는 속성이 아니라면 제거해도 좋을 것 같아용
| [components]: { | ||
| width: "2.4rem", | ||
| height: "2.4rem", | ||
| boxSizing: "border-box", |
There was a problem hiding this comment.
boxSizing: "border-box"는 이미 reset.css에서 전역적으로 적용된 값이기 때문에 제거하여도 괜찮을 것 같습니다!!
src/App.tsx
Outdated
| const DUMMY_AVERAGE_SCORE = 4.8; | ||
| const DUMMY_REVIEW_COUNT = 634; | ||
| const DUMMY_THUMBNAILS: { reviewId: number; imageUrl: string }[] = Array.from( | ||
| { length: 6 }, | ||
| (_, index) => ({ | ||
| reviewId: index + 1, | ||
| imageUrl: Product01, | ||
| }) | ||
| ); | ||
| const DUMMY_AI_SUMMARY = | ||
| "작가님의 작품은 정말 특별하고신선해요! 포장도 꼼꼼하고 배송도 빠르네요. 친구나 소중한 분께 선물하기에 딱 좋습니다. 정성스럽게 만들어주셔서 감사합니다!"; |
There was a problem hiding this comment.
후기 요약 컴포넌트는 api와 따로 연동되어 데이터를 받아오는 컴포넌트가 아니라 DUMMY_AVERAGE_SCORE, DUMMY_REVIEW_COUNT, DUMMY_THUMBNAILS, DUMMY_AI_SUMMARY와 같은 목 데이터를 사용하게 될 텐데,
이를 미리 constants나 mock 데이터 파일로 분리하여 사용하면 더 관리하기 좋을 것 같아요 👍🏻
| columnGap: "0.4rem", | ||
| rowGap: "0.4rem", |
There was a problem hiding this comment.
columnGap과 rowGap 모두 0.4rem으로 같은 값을 사용하고 있으니 gap속성 하나로 간격을 정의해줄 수 있을 것 같아요!
| {thumbnails.map((thumbnail, index) => ( | ||
| <div key={thumbnail.reviewId} className={styles.thumbnail}> | ||
| <img | ||
| src={thumbnail.imageUrl} | ||
| alt="작품 썸네일" | ||
| className={styles.thumbnailImage} | ||
| /> |
There was a problem hiding this comment.
thumbnails에 대해 map순회를 하면서 alt='작품 썸네일' 이미지를 보여주고 있는데
스크린 리더 사용자의 경우는 작품 썸네일이라는 동일한 텍스트를 반복해서 듣게돼요.
그래서 사소한 부분이지만 같은 alt로 작성해주기 보다는 접근성 개선을 위해 작품 썸네일 ${thumbnail.reviewId}나 작품 썸네일 ${index}로 수정해주면 좋을 것 같아요
jstar000
left a comment
There was a problem hiding this comment.
semantic 태그도 적절히 잘 사용해주셨고, Props 인터페이스 구성도 좋습니다!
이미지도 map + key값 사용으로 잘 구현하셨네요ㅎㅎ
남겨둔 리뷰와 suggestion으로 제안해둔 코드 확인 부탁드립니다~ 고생하셨어요!
| [components]: { | ||
| width: "2.4rem", | ||
| height: "2.4rem", | ||
| boxSizing: "border-box", |
There was a problem hiding this comment.
| boxSizing: "border-box", |
reset.css.ts 파일은 개발 시 브라우저마다 다른 기본 스타일을 초기화하기 위해 사용해요.
현재 shared/styles/reset.css.ts에
globalStyle(":where(pre)", {
"@layer": {
[layers.reset]: {
all: "revert",
boxSizing: "border-box",
},
},
});와 같이 boxSizing을 border-box로 이미 세팅해둬서 개별 컴포넌트를 구현할 때 해당 속성을 적용할 필요는 없어요! 이미 기본으로 적용돼있습니다ㅎㅎ
| [components]: { | ||
| width: "100%", | ||
| aspectRatio: "1 / 1", // 정사각형 | ||
| borderRadius: "0.3rem", |
There was a problem hiding this comment.
| borderRadius: "0.3rem", | |
| borderRadius: "3px", |
크기 단위를 rem으로 통일했지만, border-radius는 의도적으로 px로 적용해요!
rem을 사용하는 목적은 사용자가 브라우저의 기본 글꼴 크기를 조정할 때, rem 단위를 사용하는 요소들도 함께 비례해서 조정하는 것이에요. 이를 통해 웹 accessibility를 향상시킬 수 있어요.
반면 px은 절댓값으로 브라우저 환경과 관계없이 일정한 값을 유지해요. 일반적으로 컴포넌트의 테두리 굵기(border), 곡률(borderRadius) 등 사용자의 글꼴 크기 설정과 관계없이 일정하게 유지되는게 시각적으로 분명하고 디자인 의도를 살릴 수 있는 속성은 px로 값을 설정해요.
px로 값을 사용하는 다른 속성들로는 box-shadow, position(요소를 화면의 고정된 위치에 배치해야 할 때 top: 20px같이 사용) 등이 있어요!
| display: "flex", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| borderRadius: "0.3rem", |
There was a problem hiding this comment.
| borderRadius: "0.3rem", | |
| borderRadius: "3px", |
빌드 결과빌드 성공 🥳 |
📌 Summary
close feat: 후기 요약 컴포넌트 구현 #50
후기 요약 컴포넌트를 추가했습니다.
평균 별점, 후기 개수, AI 후기 요약, 후기 이미지 썸네일을 한 섹션에서 볼 수 있도록 구성했습니다.
📄 Tasks
후기 요약 컴포넌트
src/pages/review-summary/review-summary.tsxReviewSummary컴포넌트 구현averageScore,reviewCount,aiSummary,thumbnails를 props로 받아서 표시src/pages/review-summary/review-summary.css.ts별 아이콘 간격 조절
첫 번째 별까지 같이 밀리고 간격이 균일하지 않은 문제가 있었습니다......................
starIcon스타일에서selectors를 사용해서 두 번째 별부터(:not(:first-child))만 margin을 주도록 수정했습니다.→ 이 방식으로 첫 번째 별은 제자리에 두고, 두 번째 별부터만 -5px 겹치게 해서
피그마에 나온 것처럼 자연스럽게 겹치는 형태로 맞췄습니다.
마지막 썸네일 “더보기” 오버레이 처리
🔍 To Reviewer
📸 Screenshot