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: RetroPickerModal 구현 #25

Merged
merged 2 commits into from
Nov 12, 2024
Merged

feat: RetroPickerModal 구현 #25

merged 2 commits into from
Nov 12, 2024

Conversation

Klomachenko
Copy link
Collaborator

🎟️ Related Ticket: #19

✏️ Description

  • RetroPickerModal을 figma에 맞게 구현하였습니다.

🧩 How

  • 해당 Modal은 회고록 템플릿 생성 페이지 구간에서 사용됩니다.
  • 내부에 들어가는 회고 타입, 설명, 버튼 이름의 경우 변경의 소지가 적어, 상수화를 진행하였습니다.
    • constants폴더 내 retroType.ts 파일에서 확인이 가능합니다.
export const RETRO_TYPE = {
  KPT: {
    title: 'KPT',
    subTitle: 'Keep-Problem-Try',
    content: `Keep(잘한점, 유지할 점), 
              Problem(문제점, 개선이 필요한 점), 
              Try(앞으로 시도해볼 점)으로 나누어 
              회고하는 방식`,
    buttonText: 'KPT Template',
  },
  CSS: {
    title: 'CSS',
    subTitle: 'Continue-Stop-Start',
    content: `지속하거나 유지해야 할 것(Continue), 
              그만두어야 할 것(Stop), 
              새롭게 시작할 것(Start)으로 나누어 
              회고하는 방식`,
    buttonText: 'CSS Template',
  },
  FourLs: {
    title: '4Ls',
    subTitle: 'Liked-Learned-Laked-Loged for',
    content: `프로젝트를 진행하며 좋았던 부분 및 지속되었으면 하는 점(Liked),
              프로젝트를 진행하며 배운 점(Learned),
              프로젝트 중 부족했던 점(Laked),
              프로젝트에서 얻고 싶었던 점(Loged for)으로 나누어 회고하는 방식`,
    buttonText: '4Ls Template',
  },
};
  • 상수는 아래와 같이 사용합니다.
const RetroType = RETRO_TYPE;

interface RetroPickerModalProps {
  retroType: keyof typeof RetroType;
}

const RetroPickerModal: React.FC<RetroPickerModalProps> = ({ retroType }) => {
  const { title, subTitle, content, buttonText } = RetroType[retroType];

  return (
    <Container>
      <RetroTypeBox>
        <RetroTypeTitle>{title}</RetroTypeTitle>
        <RetroTypeSubTitle>{subTitle}</RetroTypeSubTitle>
      </RetroTypeBox>
      <RetroTypeContentBox>{content}</RetroTypeContentBox>
      <ButtonBox>
        <DefaultButton
          width='10.25rem'
          height='3.125rem'
          text={buttonText}
          fontSize='1.3125rem'
        />
      </ButtonBox>
    </Container>
  );
};

⚠️ PR 참고 사항

  • 원래는 state로 kpt, css, 4ls를 받아 props로 넘겨주는 방식으로 진행하려고 하였으나, 변경이 거의 없을 사항인 듯 하여, 해당 방식으로 state 없이 진행하였습니다.

📸 Screen Shot

✅� Todo Check

  • RetroPickerModal 컴포넌트 구현

💬�리뷰 요구사항

  • 적절한 상수화가 이뤄졌는제 확인해주세요. @yunn23.
    • state로 처리를 하려다가 해당 방식으로 처리해보았습니다.

Etc.

  • 이전과 동일하게 서윤님의 경우에도 상수화를 진행할 부분이 있는지 확인해보시면 좋을 것 같습니다!

@Klomachenko Klomachenko added ticket 티켓 이슈 feat 기능 구현 labels Nov 12, 2024
@Klomachenko Klomachenko added this to the 이규민 milestone Nov 12, 2024
@Klomachenko Klomachenko requested a review from yunn23 November 12, 2024 08:10
@Klomachenko Klomachenko self-assigned this Nov 12, 2024
Copy link

cr-gpt bot commented Nov 12, 2024

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

);
};

export default RetroPickerModal;

Choose a reason for hiding this comment

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

이 코드는 스타일드 컴포넌트를 사용하여 레트로 피커 모달을 만드는 것으로 보입니다.

버그 리스크와 개선 제안:

  1. RetroTypeBox 컴포넌트의 높이가 정확히 5rem이 되도록하려면 flex 방향 설정이 올바른지 확인해야 합니다.
  2. RetroTypeContentBox 컴포넌트에서 padding 속성이 두 번 정의되어 있습니다. 중복된 속성을 처리해야 합니다.
  3. 버튼의 너비와 높이를 특정 픽셀로 설정하는 것보다는 해당 속성을 상대적인 값으로 지정하는 것이 더 유연할 수 있습니다.
  4. RetroTypeContentBox의 텍스트 컨텐츠를 기준으로 높이가 자동으로 조절되도록 설정하는 것이 좋을 수 있습니다.

이외에는 코드가 깔끔하고 구조화가 잘 되어 있는 것으로 보입니다. 개인적으로 작은 스타일적인 조정을 위해 픽셀을 사용하는 대신에 상대적인 값 및 자동 크기 조절을 더 고려해볼 수 있겠습니다.

프로젝트에서 얻고 싶었던 점(Loged for)으로 나누어 회고하는 방식`,
buttonText: '4Ls Template',
},
};

Choose a reason for hiding this comment

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

이 코드 패치는 RETRO_TYPE 상수 객체를 내보내는 것으로 시작합니다. 각 항목은 다른 회고 방법을 나타내며, 각각의 title, subTitle, content 및 buttonText 속성을 가지고 있습니다.

코드에서 발견된 잠재적 버그나 위험성은 없어 보입니다. 그러나 content 속성의 문자열에 개행 문자 및 들여쓰기가 잘못된 부분이 있습니다. 여러 줄의 문자열은 백틱(`)을 사용하여 표시되는데, 들여쓰기를 일관되게 적용하는 것이 좋습니다.

또한, content 속성은 템플릿이 자세히 설명되어 있지만, buttonText 속성만으로는 해당 템플릿을 어떻게 사용해야 하는지에 대한 안내가 부족합니다. 이를 보완하기 위해 각 회고 방법에 대한 간단한 사용 설명서 또는 가이드가 추가될 수 있습니다.

개선 제안으로는 RETRO_TYPE 객체에 새로운 회고 방법을 확장하는 경우를 고려하여 유연성을 높이는 것이 좋을 것입니다. 또한, 각 회고 방법에 대한 문서화 또는 주석 추가도 도움이 될 수 있습니다.

<RetroNoticeModal />
<RetroPickerModal retroType='KPT' />
<RetroPickerModal retroType='FourLs' />
<RetroPickerModal retroType='CSS' />
</div>
);
};

Choose a reason for hiding this comment

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

이 코드 패치는 RetroNoticeModal을 RetroPickerModal로 대체하고 세 가지 다른 retroType props와 함께 세 개의 RetroPickerModal을 랜더링합니다. 이 코드의 주요 리스크는 retroType 값이 올바르지 않을 수 있고 존재하지 않는 경우 컴포넌트가 오동작 할 수 있습니다. 이 문제를 피하기 위해 retroType props가 올바른 값인지 확인하는 로직을 추가하는 것이 좋을 것입니다. 또한, Klomachenko 함수의 이름이 특이하게 보일 수 있으므로 함수의 목적을 명확하게 설명하는 이름으로 변경하는 것이 가독성을 향상시킬 수 있습니다.

@Klomachenko Klomachenko merged commit 7039243 into develop Nov 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 기능 구현 ticket 티켓 이슈
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant