-
Notifications
You must be signed in to change notification settings - Fork 3
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: 멤버 탭 변경사항 및 mds 반영 #1705
base: main
Are you sure you want to change the base?
feat: 멤버 탭 변경사항 및 mds 반영 #1705
Conversation
현재 Select의 오픈 상태에 따른 스타일링을 반영할 수 없어, mds 수정 사항 요청 상태
|
🚀 프리뷰 배포 확인하기 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생했어!! 깔끔한 코드 배워갑니다!!
@@ -79,7 +79,7 @@ export default function CoffeechatDetail({ memberId }: CoffeechatDetailProp) { | |||
isMine={profile.isMine} | |||
isCoffeechatTap | |||
/> | |||
<DetailInfoSection profile={profile} isCoffeechat /> | |||
<DetailInfoSection profile={profile} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기 없애준 이유가 뭘까요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<DetailInfoSection/>
컴포넌트는 멤버 프로필과 커피챗에서 재사용되는 컴포넌트인데요! 기존에 이 섹션 내에서 유저의 생일 정보를 노출하고 있었는데, 이번에 생일 정보를 다른 위치로 옮기는 변경사항이 생겼어요.
그런데 <DetailInfoSection/>
컴포넌트에서 isCoffeeChat이라는 flag를 통해 생일 정보를 조건부 렌더링하고 있었고, 위의 변경사항에 따라 더이상 이 분기가 필요하지 않아서 isCoffeeChat이라는 prop이 필요가 없어졌습니다!
기존의 <DetailInfoSection/>
내 조건부 렌더링
{!isCoffeechat && profile.birthday && (
<InfoItem label='생년월일' content={convertBirthdayFormat(profile.birthday)} />
)}
-> 더이상 이 컴포넌트에서 생일을 노출하지 않으므로 해당 코드 아예 삭제
{String(meId) === memberId ? ( | ||
<> | ||
{String(meId) === memberId && ( | ||
<Container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요부분 훨씬 직관성 있게 바뀐 것 같아요!! 배워갑니다!
{profiles.map((profile) => { | ||
const sorted = profile.activities.sort((a, b) => b.generation - a.generation); | ||
const badges = sorted.map((activity) => ({ | ||
content: `${activity.generation}기 ${activity.part}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(아주 사소한 개인의 의견) profile 자체에 null이나 빈배열이 있거나, 대부분의 경우에는 정상 코드일 확률이 높지만.. 라이프사이클 전체를 제어할 수가 없는 배열의 객체의 경우에는 activity?.generation 처럼 예외처리를 해주는게 어떨까 싶기는해요! 이건 개인의 코드 스타일인거라 논의거리로 남겨보는거긴한데.. 저는 라이프사이클을 본인이 전부 관리하는 코드가 아니라면 예외 케이스를 모두 고려하는걸 목표로 하곤 있습니당..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 제가 잘 이해했는지 확인하고자 여쭤보는건데,
activity.generation
, activity.part
가 빈 값일 경우 content 값이 이상해질 수 있으니 아예 아래와 같이 예외처리를 하면 좋겠다는 말씀이실까요??
const badges = sorted
.filter(activity => activity.generation && activity.part) // 값이 올바르지 않을 경우 아예 badges 배열에서 제외
.map((activity) => ({
content: `${activity.generation}기 ${activity.part}`,
isActive: activity.generation === LATEST_GENERATION,
}));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 맞습니다!! 사실 저조차도 잘 못지키는 원칙이지만...데이터의 라이프사이클을 고려하면 좀 더 안정성 있는 구조가 아닐까 했어요!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
activity?.generation
라고 예시를 적어주신 것은 activity가 비어있을 경우를 방어하기 위함인 것 같은데, sorted는 이미 배열임이 보장되어 있고 이 배열의 항목인 activity의 존재 여부를 방어하는 것이 잘 이해가 되지 않습니다! 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 제가 처음에 남긴 코멘트처럼 필터링을 한번 해주는 것은 매우 타당한거같습니다!!! 감사해요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 맞습니다!!!! 첫번째로 답 남겨준것 처럼 프로퍼티들에 대한 방어로직 얘기였어요!! 제가 좀 더 진취적으로 코드리뷰 해보려다가 말이 조금 꼬여버렸어요 죄송합니다 ㅋㅋㅋㅋㅋ..... 첫번째 댓글을 의도하고 말씀드린게 맞아요!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 알겠습니다!! ㅋㅋㅋㅋㅋ 섬세하게 봐주셔서 너무 감사드려요!!!! ✨👍👍👍👍👍
도영이가 만들어준 BottomSheetSelect를 멤버 탭에 적용하면서 수정해야할 부분이 있었는데, 1. 빈 문자열 value 처리를 위한 조건부 로직 수정{selectedValue !== null ? <p>{selectedValue}</p> : <p style={{ color: '#808087' }}>{placeholder}</p>} 기존에 선택된 값을 표시할 때 즉 '전체' 옵션을 선택하면 selectedValue가 {selectedValue ? <p>{selectedValue}</p> : <p style={{ color: '#808087' }}>{placeholder}</p>} 동일한 이유로 handleConfirm에서 temporaryValue가 빈 문자열인지 체크하는 로직도 제거하게 됐어요. const handleConfirm = () => {
setSelectedValue(temporaryValue);
// if (temporaryValue !== '') onChange(temporaryValue as string); 조건문 삭제
onChange(temporaryValue as string)
handleClose();
}; 혹시 빈 문자열에 대한 예외처리를 해주어야했던 특별한 이유가 있었을까요?? 2. defaultOption prop추가앞서 언급한 '전체' 옵션과 같은 defaultOption을 지정해줄 필요가 있었어요. mds에서의 Select에서도 defaultOption prop을 받아서 사용하고 있기 때문에 사용 방식의 일관성을 맞추고자 BottomSheetSelect에도 defautOption을 추가하여 사용할 수 있도록 했어요! <OptionList>
// defaultOption을 선택지에 보여줌 -->
{defaultOption && (
<OptionItem onClick={() => handleOptionSelect(defaultOption.value)}>
{defaultOption.label}
{temporaryValue === defaultOption.value && <CheckedIcon />}
</OptionItem>
)} // <--
{options.map((option) => (
<OptionItem key={option.value} onClick={() => handleOptionSelect(option.value)}>
{option.label}
{temporaryValue === option.value && <CheckedIcon />}
</OptionItem>
))}
</OptionList> 3. InputField에서 value가 아닌 label을 보여주도록 수정현재 선택된 값을 보여주는 영역에서 {selectedValue}를 보여주고 있었어요. 그런데 여기서 value가 아닌 label을 보여주어야해요. export const CAREER_LEVEL_OPTIONS = [
{ label: '아직 없어요', value: '아직 없어요' },
{ label: '인턴 경험만 있어요', value: '인턴 경험만 있어요' },
...
] as const; 실제로 label과 value를 나눠서 관리하는 이유는 내부적으로 관리해야하는 값(value)과 사용자에게 보여주어야할 값(label)이 다른 경우가 있기 때문인데요! 커피솝 오픈 쪽에서는 그럴 필요가 없어서 문제가 없었던 것 같지만 멤버 탭에서는 value와 label이 다른 값들이 존재해요! 설명이 길었지만 아무튼 이러한 이유로, selectedValue를 그대로 표시하는 것이 아니라 이 value에 해당하는 label을 보여주도록 다음과 같이 수정했어요! const getSelectedLabel = (value: string) => {
return options.find((option) => option.value === value)?.label;
};
...
{selectedValue ? <p>{getSelectedLabel(selectedValue)}</p> 4. value type에서 string[] 제거BottomSheetSelectProps에서 value의 타입이 4. 스타일 관련 수정
우선 이러한 수정사항으로 커피솝 오픈/수정 테스트해봤을 때 문제는 없어보였습니다!! |
1번과 같은 부분에서는 제가 놓친 것 같아요! 감사합니다 ㅎ.ㅎ 2,3 번은 확인했습니다! 4번의 경우에는 type으로 올 수 있는 value 를 string[] 을 추가를 해주지 않으면 오류가 발생해서 추가해주었습니다! 모두 확인했습니다. 열심히 수정해주셔서 감사합니다!!! ㅎ.ㅎ |
🤫 쉿, 나한테만 말해줘요. 이슈넘버
🧐 어떤 것을 변경했어요~?
멤버 탭 자잘한 변경사항
mds 반영
🤔 그렇다면, 어떻게 구현했어요~?
멤버 상세에서 '생일'항목이 번호,이메일과 함께 들어가게 되면서 레이아웃 스타일링이 변경된 것이 가장 큰 변화입니다.
그 외에 자잘한 css 변경과 mds 교체 작업을 진행했습니다.
❤️🔥 당신이 생각하는 PR포인트, 내겐 매력포인트.
mds 반영 필요건
멤버 리스트 정렬 드롭다운에서 Select의 기본 화살표가 아닌 커스텀 아이콘을 사용해야하는 상황입니다. 이를 위해서는 SelectV2.TriggerContent에 children으로 커스텀 컴포넌트를 넣는 등의 방법이 필요해보이는데, 일단 children 허용이 안되어있어서 요청 드린 상태입니다. -> 수정 완료
새로 추가된 mds icon (phone, birthday)에 대해 color 주입이 불가능한 상태라 (currentColor 지정이 안되어있음) 이또한 수정 요청 드린 상태입니다. -> 수정 완료
해당 작업 내용 반영이 급한 사항이 아니므로 mds가 반영되고 나서 머지하는게 어떨까 합니다!
바텀시트 관련
도영이가 올린 커피챗 수정 PR을 보니 mds용 바텀시트를 너무 잘 구현해주어서.... 저도 가져다 써야 할 것 같습니다 ㅎㅎ 그래서 도영이 PR 먼저 머지된 후 가져와서 반영하도록 하겠습니다.
📸 스크린샷, 없으면 이것 참,, 섭섭한데요?
현재 반영된 멤버 리스트 정렬 Select
실제로 반영되어야 할 모습
멤버 리스트 정렬 Select -> mds 커스텀 어려움 이슈로 현재 선택시 피그마대로 흰색 border처리가 안되는 상황입니다.
변경된 ProfileSection
기존 디자인
변경된 디자인
위에 언급한대로 phone, birthday icon에 대해 색상 지정이 불가능하여 mds 아이콘 수정이 필요합니다.
변경된 SoptActivitySection
기존 디자인
변경된 디자인