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: 멤버 탭 변경사항 및 mds 반영 #1705

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

simeunseo
Copy link
Member

@simeunseo simeunseo commented Dec 13, 2024

🤫 쉿, 나한테만 말해줘요. 이슈넘버

🧐 어떤 것을 변경했어요~?

멤버 탭 자잘한 변경사항

  • 참여한 프로젝트, 모임이 없는 경우 해당 항목 전부 비노출
  • 본인이 커피챗 오픈 안했을 경우 프로필에 커피챗 오픈 유도 버튼 추가

mds 반영

  • mds ui, icon 버전 업 및 개선된 Select에 맞추어 기존의 임시 레이아웃 돌려놓기
  • 멤버 리스트 정렬 드롭다운을 mds Select로 교체
  • 멤버 상세 내 최상단 프로필 영역 레이아웃 변경 및 생일 정보 위치 이동
  • 멤버 상세 내 SOPT 활동 영역 레이아웃 변경, 뱃지 스타일 변경, mds 태그 반영

🤔 그렇다면, 어떻게 구현했어요~?

멤버 상세에서 '생일'항목이 번호,이메일과 함께 들어가게 되면서 레이아웃 스타일링이 변경된 것이 가장 큰 변화입니다.
그 외에 자잘한 css 변경과 mds 교체 작업을 진행했습니다.

❤️‍🔥 당신이 생각하는 PR포인트, 내겐 매력포인트.

mds 반영 필요건

  • 멤버 리스트 정렬 드롭다운에서 Select의 기본 화살표가 아닌 커스텀 아이콘을 사용해야하는 상황입니다. 이를 위해서는 SelectV2.TriggerContent에 children으로 커스텀 컴포넌트를 넣는 등의 방법이 필요해보이는데, 일단 children 허용이 안되어있어서 요청 드린 상태입니다. -> 수정 완료

  • 새로 추가된 mds icon (phone, birthday)에 대해 color 주입이 불가능한 상태라 (currentColor 지정이 안되어있음) 이또한 수정 요청 드린 상태입니다. -> 수정 완료

해당 작업 내용 반영이 급한 사항이 아니므로 mds가 반영되고 나서 머지하는게 어떨까 합니다!

바텀시트 관련

도영이가 올린 커피챗 수정 PR을 보니 mds용 바텀시트를 너무 잘 구현해주어서.... 저도 가져다 써야 할 것 같습니다 ㅎㅎ 그래서 도영이 PR 먼저 머지된 후 가져와서 반영하도록 하겠습니다.

📸 스크린샷, 없으면 이것 참,, 섭섭한데요?

현재 반영된 멤버 리스트 정렬 Select

image

실제로 반영되어야 할 모습

image

멤버 리스트 정렬 Select -> mds 커스텀 어려움 이슈로 현재 선택시 피그마대로 흰색 border처리가 안되는 상황입니다.

변경된 ProfileSection

기존 디자인

image

변경된 디자인

image
image

위에 언급한대로 phone, birthday icon에 대해 색상 지정이 불가능하여 mds 아이콘 수정이 필요합니다.

변경된 SoptActivitySection

기존 디자인

image

변경된 디자인

image
image

@simeunseo simeunseo added the ✨ Feature 신규 피쳐 label Dec 13, 2024
@simeunseo simeunseo self-assigned this Dec 13, 2024
Copy link

height bot commented Dec 13, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

github-actions bot commented Dec 13, 2024

✨✨ 스토리북으로 확인하기 ✨✨

Copy link

github-actions bot commented Dec 13, 2024

🚀 프리뷰 배포 확인하기 🚀

https://a0f17908.sopt-internal-dev.pages.dev

Copy link
Contributor

@hayounSong hayounSong left a 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} />
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

@simeunseo simeunseo Dec 16, 2024

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>
Copy link
Contributor

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}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

(아주 사소한 개인의 의견) profile 자체에 null이나 빈배열이 있거나, 대부분의 경우에는 정상 코드일 확률이 높지만.. 라이프사이클 전체를 제어할 수가 없는 배열의 객체의 경우에는 activity?.generation 처럼 예외처리를 해주는게 어떨까 싶기는해요! 이건 개인의 코드 스타일인거라 논의거리로 남겨보는거긴한데.. 저는 라이프사이클을 본인이 전부 관리하는 코드가 아니라면 예외 케이스를 모두 고려하는걸 목표로 하곤 있습니당..!

Copy link
Member Author

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,
  }));

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.

activity?.generation라고 예시를 적어주신 것은 activity가 비어있을 경우를 방어하기 위함인 것 같은데, sorted는 이미 배열임이 보장되어 있고 이 배열의 항목인 activity의 존재 여부를 방어하는 것이 잘 이해가 되지 않습니다! 😭

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
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.

아아 알겠습니다!! ㅋㅋㅋㅋㅋ 섬세하게 봐주셔서 너무 감사드려요!!!! ✨👍👍👍👍👍

@simeunseo
Copy link
Member Author

simeunseo commented Dec 19, 2024

@pepperdad

도영이가 만들어준 BottomSheetSelect를 멤버 탭에 적용하면서 수정해야할 부분이 있었는데,
한번 확인해주어야할 것 같아서 수정사항 정리해봤어요!

코드 상으로는 커밋1, 커밋2 확인해주면 돼요!!!

1. 빈 문자열 value 처리를 위한 조건부 로직 수정

{selectedValue !== null ? <p>{selectedValue}</p> : <p style={{ color: '#808087' }}>{placeholder}</p>}

기존에 선택된 값을 표시할 때 selectedValue !== null 과 같이 검사하고 있었어요.
그런데 멤버 탭에서 Select를 사용할 때 선택한 값을 초기화하기 위해 '전체' 옵션을 선택하면 빈 문자열을 넘기게 되어있어요.
(빈 문자열이 아닌 null과 같은 값을 사용하게되면 Select를 사용할 때 제어상태 일관성을 유지할 수 없기 때문)

즉 '전체' 옵션을 선택하면 selectedValue가 ''이 되면서 선택한 값이 초기화되어 placeholder를 보여주어야해요.
따라서 빈 문자열도 조건에 걸릴 수 있도록 아래와 같이 조건문을 변경하였어요.

{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을 보여주어야해요.
커피솝 오픈 쪽 코드를 확인해보니 option의 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의 타입이 string | string[] | null | undefined으로 되어있었는데,
여기서 string[]이 허용되는 건 바텀시트에서 여러 옵션을 선택하는 경우를 위한 걸까요?
이 부분은 충분히 대응할만한 지점이긴하지만 선택값이 여러개일 때 UI를 어떻게 보여주어야할지에 대한 명세가 없기 때문에
그에 대한 처리는 추후 mds쪽에 맞기고 일단 플그에서는 단일 string만 처리해도 되지 않을까 해요..!
왜냐면 앞서 3번에서 언급했듯 현재 선택된 값인 selectedValue에 대한 label을 찾아서 보여주기 위해서는 string[]을 허용할 수가 없거든요,,

4. 스타일 관련 수정

  1. 여러개의 BottomSheetSelect를 나열해서 사용했을 때 레이아웃이 깨지는 부분들이 있어 수정했어요
  2. 멤버탭의 정렬 부분의 경우 기본 화살표가 아니라 커스텀 아이콘을 사용해야하고, 스타일이 조금 다른 부분이 있어서 icon, className prop을 추가했어요

우선 이러한 수정사항으로 커피솝 오픈/수정 테스트해봤을 때 문제는 없어보였습니다!!

@pepperdad
Copy link
Member

@simeunseo

1번과 같은 부분에서는 제가 놓친 것 같아요! 감사합니다 ㅎ.ㅎ

2,3 번은 확인했습니다!

4번의 경우에는 type으로 올 수 있는 value 를 string[] 을 추가를 해주지 않으면 오류가 발생해서 추가해주었습니다!
은서가 수정했는데, 오류가 발생하지 않았다면 없애도 괜찮을 것 같아요! 단일 string만 올 수 있다고 생각합니다!

모두 확인했습니다. 열심히 수정해주셔서 감사합니다!!! ㅎ.ㅎ

@simeunseo simeunseo added this pull request to the merge queue Dec 24, 2024
@simeunseo simeunseo removed this pull request from the merge queue due to a manual request Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 멤버 프로필 mds 적용 및 수정사항 반영
3 participants