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: Profile페이지 명상정보 컴포넌트 추가 #23

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

hyesung99
Copy link
Collaborator

🪄 변경사항

MeditaionInfo, MeditaionInfoItem 컴포넌트 두개 추가했습니다.

🖥 결과 화면

image

✏️ PR 포인트

<MeditationInfoItem icon='🧘🏻'>
    <p>
      <strong>{fullName}</strong> 님은 총 <b>{totalMeditationCount}</b>번의
      명상을 진행했어요.
    </p>
</MeditationInfoItem>

icon은 prop으로 주고 '~은 ~번했습니다'에 해당하는 타이틀은 children으로 넘겨줍니다
fullName, totalMeditationCount같은 prop이 너무 많아지기도 하고 타이틀을 string으로 넘겨주면 특정 문자에만 스타일을 주기가 힘들어서... p태그를 children으로 넘겨줬는데 통일성이 없어보이기도 하네요? 더 좋은 방법이 있을까요?

그리고 strong, b태그는 태그의 속성은 안쓰고 className처럼 썻는데 이런 꼼수말고 진짜 span에 className을 줘서 특정 문자에 스타일을 주는게 좋을까요?

@hyesung99 hyesung99 self-assigned this Sep 12, 2023
Copy link
Member

@sscoderati sscoderati left a comment

Choose a reason for hiding this comment

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

LGTM! 리뷰 완료입니다~

+) strong / b 태그 관련
이미 컴포넌트의 depth가 깊어져서 fullName과 totalMeditation을 감싸는 컴포넌트의 이름 또는 span에 할당될 className이 매우매우 길어질 것 같아요! 이런 경우 의미는 확실해도 가독성에 안 좋은 영향을 줄 것 같아서 저는 이런 태그 활용 좋다고 생각합니다~!

Comment on lines 14 to 19
<MeditationInfoItemContainer>
<MeditationInfoItemIconSection>{icon}</MeditationInfoItemIconSection>
<MeditationInfoItemTitleSection>
{children}
</MeditationInfoItemTitleSection>
</MeditationInfoItemContainer>
Copy link
Member

Choose a reason for hiding this comment

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

어제 핫 포테이토였던 Section과 Container... 실제로 보니 반갑네요 허허

Suggested change
<MeditationInfoItemContainer>
<MeditationInfoItemIconSection>{icon}</MeditationInfoItemIconSection>
<MeditationInfoItemTitleSection>
{children}
</MeditationInfoItemTitleSection>
</MeditationInfoItemContainer>
<MeditationInfoItemSection>
<MeditationInfoItemIconContainer>{icon}</MeditationInfoItemIconContainer>
<MeditationInfoItemTitleContainer>
{children}
</MeditationInfoItemTitleContainer>
</MeditationInfoItemSection>

Section <-> Container 바꿔주시는건 어떤가용

Comment on lines +17 to +28
<MeditationInfoItem icon='🧘🏻'>
<p>
<strong>{fullName}</strong> 님은 총 <b>{totalMeditationCount}</b>번의
명상을 진행했어요.
</p>
</MeditationInfoItem>
<MeditationInfoItem icon='⏰'>
<p>
<strong>{fullName}</strong> 님은 총 <b>{totalMeditationTime}</b>분의
명상을 하셨어요.
</p>
</MeditationInfoItem>
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 저도 고민을 해봤는데 혜성님 방식도 통일성은 조금 떨어질지언정 가독성이 좋아서 괜찮아보이고, 제가 생각한 다른 방식은 어차피 '~~님은 총 ~~(번의 | 분의) ~~ '의 텍스트는 const textTemplate 정도로 InfoItem에서 관리하고, Prop으로 전달받는 이모지에 따라서 다르게 보여주는 식으로 구현하는 방식인데 어떤게 더 나을지는 저도 고민이 되네요 ㅎㅎ 😅

Copy link
Collaborator

@suyeon1218 suyeon1218 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hyesung99 hyesung99 merged commit 0a7b3ed into main Sep 12, 2023
@hyesung99 hyesung99 deleted the feat/profile/meditaitionInfo branch September 12, 2023 05:25
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.

3 participants