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(apply): implement the mission view page #763

Merged
merged 15 commits into from
Sep 12, 2024

Conversation

woowahan-cron
Copy link

@woowahan-cron woowahan-cron commented Aug 5, 2024

Resolves #759

해결하려는 문제가 무엇인가요?

  • As-Is: 지원자가 미션을 진행할 시 이메일로 수신한 과제 요구사항을 확인하여 과제를 작성합니다
    • To-Be: 지원자가 미션을 진행할 시 과제 요구사항을 지원플랫폼 내에서 확인할 수 있습니다. (와이어프레임)
      • image

어떻게 해결했나요?

  • 과제 보기 페이지 추가 후 신설된 서버 API 명세를 이용한 과제 미리보기 내용 수신 후 렌더링

어떤 부분에 집중하여 리뷰해야 할까요?

  • 라우팅을 비롯한 새롭게 추가된 페이지의 이름이 적절한지 검토해 주세요!
    • 과제 보기 라우팅: /assignment/:recruitmentId/mission/:missionId(기존 과제 제출하기 경로는 /assignment/new 또는 /assignment/edit)
  • 과제 보기 페이지 컴포넌트: MissionView
    • 재사용할 수 있는 컴포넌트가 아니라 단순히 페이지 컴포넌트로 재사용 목적은 없기 때문에 -er은 뺀 형태의 MissionView라는 명칭을 사용하였습니다. MissionViewer의 명칭은 재사용할 수 있는 컴포넌트로 오해할 수 있어 단순히 과제 데이터를 표시하는 목적의 명확성을 드러내기 위해 MissionView로 명명했습니다.
  • 오류 처리
    • 과제 보기 페이지(MissionViewer)에서 API 스펙에서는 현재 오류가 나는 경우가 모두 404 Error Code와 메시지를 반환하고 있습니다. 오류 처리가 적절했는지 검토해 주세요.
      • 그런데 사실 과제 보기 페이지에 직접 주소를 치고 접근하는 것이 아닌 이상, 과제를 제출할 수 있는 기한을 넘긴 경우 과제를 확인할 수 없어서 일반적인 접근으로는 이 메시지를 사용자가 직접 확인하기는 어렵습니다.

RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

@woowahan-cron woowahan-cron self-assigned this Aug 5, 2024
@woowahan-cron woowahan-cron force-pushed the feature/mission-description-view branch from 0f89920 to 0cfeb04 Compare August 6, 2024 01:08
@woowahan-cron woowahan-cron force-pushed the feature/mission-description-view branch from 0cfeb04 to 7051503 Compare August 6, 2024 01:36
@woowahan-pjs
Copy link
Contributor

  • 로컬에서 더 쉽게 테스트할 수 있도록 더미 데이터를 조금 더 풍부하게 만들었습니다.
  • 빠르게 코드 리뷰해 보자면 평가자가 작성하는 것을 과제(mission), 평가자가 제출하는 것을 과제 제출물(assignment)이라고 합니다.
    • 따라서 과제 보기 컴포넌트는 MissionViewer와 같은 이름을 가져야 합니다.

@woowahan-cron woowahan-cron force-pushed the feature/mission-description-view branch from 2a97e63 to b517f70 Compare August 12, 2024 05:30
@woowahan-cron woowahan-cron force-pushed the feature/mission-description-view branch from b517f70 to c1c0ce1 Compare August 12, 2024 06:35
@woowahan-cron woowahan-cron force-pushed the feature/mission-description-view branch from 766743a to 5be882f Compare August 14, 2024 06:13
@woowahan-cron woowahan-cron changed the title feat: 과제 보기 버튼 배치 feat: Placing task view button Aug 14, 2024
const { token } = useTokenContext();

const goBack = () => {
navigate(-1);
Copy link
Author

Choose a reason for hiding this comment

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

명시적으로 내 지원 현황 페이지인 /applications/me 경로로 라우팅하는 것이 더 적절할까요?

image
Suggested change
navigate(-1);
navigate('/applications/me'); // (예시)

Copy link
Contributor

Choose a reason for hiding this comment

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

a:

  • '뒤로 가기' 기능이 의도라면 현재가 더 적절하다고 생각합니다.
  • 내 지원현황 페이지로 명식적으로 이동시키는 것이 의도라면, 함수 이름도 그에 맞춰서 같이 바꾸는 게 더 혼동의 여지가 적다고 생각해요 :)

Copy link
Author

Choose a reason for hiding this comment

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

이 의도에 대해서는 구체적으로 논의된 적이 없어서요!
일단 '뒤로 가기'기능으로 구동하더라도 큰 문제가 되지 않으니 적용해 보고 적절치 않으면 다음 이슈에서 진행해보는 것이 좋을 것 같아요 😁

Copy link
Author

Choose a reason for hiding this comment

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

로컬 테스트를 위한 서버 더미 데이터 보완 작업

  • 로컬에서 더 쉽게 테스트할 수 있도록 더미 데이터를 조금 더 풍부하게 만들었습니다.

  • 빠르게 코드 리뷰해 보자면 평가자가 작성하는 것을 과제(mission), 평가자가 제출하는 것을 과제 제출물(assignment)이라고 합니다.

    • 따라서 과제 보기 컴포넌트는 MissionViewer와 같은 이름을 가져야 합니다.

@@ -5,6 +5,8 @@
"dependencies": {
"axios": "^0.21.1",
"classnames": "^2.3.1",
"github-markdown-css": "^5.6.1",
Copy link
Author

Choose a reason for hiding this comment

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

기존 과제 내용이 Github 저장소 README.md의 스타일링을 고려하여 작성한 내용이다 보니, 그대로 스타일을 가져오는 것이 콘텐츠 제공자와 사용자 모두의 입장에서 좋을 것이라고 생각했어요.

  • github-markdown-css 라이브러리는 Github 저장소에 적용되는 스타일을 그대로 사용할 수 있습니다.
  • 다크 모드를 지원하기 때문에 추후 플랫폼 내 다크 모드 스타일링을 도입한다면, 바로 다크 모드 스타일을 적용할 수 있습니다

Copy link
Contributor

@woowapark woowapark Aug 25, 2024

Choose a reason for hiding this comment

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

c: 해당 라이브러리를 선택하신 이유가 있을까요?
저도 정확히 같은 기능을 구현한 적은 없어서 리뷰하며 잠시 찾아보았는데, 아래와 같이 React 컴포넌트 형식으로, 코드 하이라이팅을 함께 지원하는 라이브러리도 있길래
해당 라이브러리를 최종 결정하신 이유도 궁금합니다.
https://www.npmjs.com/package/@uiw/react-markdown-preview

Copy link
Author

Choose a reason for hiding this comment

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

라이브러리를 선택한 배경에 관해 궁금해하시는 것 같습니다!
서버에서는 DB에 마크다운 형식으로 가지고 있지만, 클라이언트에 전달할 때는 html로 변환해서 던져주게 됩니다.
그래서 이 상황에 적합한 라이브러리는 github-markdown-css 라고 판단했습니다 🙂

@@ -5,6 +5,8 @@
"dependencies": {
"axios": "^0.21.1",
"classnames": "^2.3.1",
"github-markdown-css": "^5.6.1",
"highlight.js": "^11.10.0",
Copy link
Author

Choose a reason for hiding this comment

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

그런데 github-markdown-css는 코드 블록 스타일링은 지원하지 않아서 추가적으로 highlight.js 라이브러리를 추가적으로 도입하게 되었습니다.

우아한테크코스에서 사용하는 Java, Javascript, Kotlin의 언어 모두 지원합니다(언어 지원 목록).

@@ -8,7 +8,7 @@ type MissionProps = {
recruitmentId: string;
};

const missionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
export const getMissionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
Copy link
Author

Choose a reason for hiding this comment

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

이 함수는 MissionView.tsx에서도 쓰입니다. 이 함수가 지금 useMission 훅에 존재하는 것이 적절하지는 않아보이는데 마땅히 어느 곳에 있어야 할지 판단이 안 서네요😓

Copy link
Contributor

Choose a reason for hiding this comment

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

a:

  • MissionView.tsx에서 쓰이는 것 때문에 현재 위치가 적절해보이지 않는다는 의미이실까요? 어떤 이유에서 그렇게 보게 되셨는지 궁금합니다.
  • 다만 적절함 유무와 별개로 이 함수의 이름을 변경하는 것은 refactor 작업이라고 생각해요. 현재 PR은 과제 보기 기능 구현이기 때문에 수정한다면 별도 작업에서 진행해도 되지 않을까 의견 남겨둡니당

Copy link
Author

Choose a reason for hiding this comment

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

  • 미션 제출 상태에 따라 적합한 레이블 텍스트를 뽑아 내는 함수인데 이것이 훅이 가지고 있는 함수라기보다는, 유틸 함수로 빼야 하는 것 아닌가? 하는 고민에서 시작되었습니다.
  • 필요하다면, 별도 이슈에서 작업할게요~

@woowahan-cron woowahan-cron marked this pull request as ready for review August 14, 2024 06:18
Copy link
Contributor

@woowapark woowapark left a comment

Choose a reason for hiding this comment

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

코멘트는 질문들로만 남겨두었습니다 🙏
작업 관리 측면에서는 우선 반영하고, 추가 수정이 필요한 경우 별도 PR로 작업하는 것이 좋지 않을까 의견 같이 남겨두어요!
고생하셨습니다~~! 🙇‍♂️🙇‍♂️🙇‍♂️

missionItem.status === MISSION_STATUS.ENDED ||
missionItem.status === MISSION_STATUS.UNSUBMITTABLE
}
onClick={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 리턴하는 JSX에서 onClick 핸들러를 인라인으로 정의하는 경우 매 렌더마다 새로운 함수가 생성됩니다. 이후에 개선한다면 별도 핸들러로 분리하는 것을 권장드립니다.

Copy link
Author

Choose a reason for hiding this comment

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

반영해둘게요~

className={buttonStyles["assignment-button"]}
disabled={
missionItem.status === MISSION_STATUS.ENDED ||
missionItem.status === MISSION_STATUS.UNSUBMITTABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 이슈에는 과제의 상태가 SUBMITTING이 아닌 경우 버튼 비활성화로 되어 있던데, 이 명세 그대로라면 missionItem.status !== MISSION_STATUS.SUBMITTING 으로만 설정해도 되는 것은 아닌가요?-?

Copy link
Author

Choose a reason for hiding this comment

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

좋은 방법 알려주셔서 감사해요 ㅋㅋ
제가 제출 상태에 대한 이해도가 부족했어서요!

@@ -30,7 +30,7 @@ export type FetchMyMissionJudgmentResponseData = Judgment;
export type FetchMyMissionsResponseData = Mission[];

export type FetchAssignmentRequest = RequestWithToken<{
recruitmentId: number;
recruitmentId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 타입이 변경된 걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

요청을 먼저 날려서 되는지를 확인하려고 타입을 잠시 바꿔둔 것이었는데 나중에 후속 작업을 누락했던 것으로 추정하고 있습니다.
다시 변경해 두었습니다! 그런데 코드 곳곳에 string으로 선언된 부분들도 있어서 나중에 리팩터링할 때 형식을 통일하는 작업이 필요해 보입니다

@@ -8,7 +8,7 @@ type MissionProps = {
recruitmentId: string;
};

const missionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
export const getMissionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

a:

  • MissionView.tsx에서 쓰이는 것 때문에 현재 위치가 적절해보이지 않는다는 의미이실까요? 어떤 이유에서 그렇게 보게 되셨는지 궁금합니다.
  • 다만 적절함 유무와 별개로 이 함수의 이름을 변경하는 것은 refactor 작업이라고 생각해요. 현재 PR은 과제 보기 기능 구현이기 때문에 수정한다면 별도 작업에서 진행해도 되지 않을까 의견 남겨둡니당

@@ -5,6 +5,8 @@
"dependencies": {
"axios": "^0.21.1",
"classnames": "^2.3.1",
"github-markdown-css": "^5.6.1",
Copy link
Contributor

@woowapark woowapark Aug 25, 2024

Choose a reason for hiding this comment

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

c: 해당 라이브러리를 선택하신 이유가 있을까요?
저도 정확히 같은 기능을 구현한 적은 없어서 리뷰하며 잠시 찾아보았는데, 아래와 같이 React 컴포넌트 형식으로, 코드 하이라이팅을 함께 지원하는 라이브러리도 있길래
해당 라이브러리를 최종 결정하신 이유도 궁금합니다.
https://www.npmjs.com/package/@uiw/react-markdown-preview

try {
if (!recruitmentId || !missionId) {
goBack();
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 이 부분은 url을 직접 입력해서 들어왔는데 잘못된 경로일 경우를 대비한 부분일까요? + fetchRequirement라면 API 호출만을 책임지는 게 맞을 것 같아요. goBack() 같은 내비게이션 동작은 분리해내는 게 더 명확할 것 같아 코멘트만 남겨둡니다.

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
Author

Choose a reason for hiding this comment

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

goBack() 같은 내비게이션 동작은 분리해내는 게 더 명확할 것 같아 코멘트만 남겨둡니다.
3주 뒤에 코드를 다시 보니 명확해 보이지 않는다는 피드백에 동감합니다 😅

처음 의도는 뒤에 이어지는 useEffect에 동일한 방어 로직(if (!recruitmentId || !missionId))을 작성해야 하는데 중복되는 코드를 작성해야 하나 생각했었거든요. 겹치더라도 분리해 내는 것이 적절해 보여서 반영해 두겠습니다!

 useEffect(() => {
    if (!recruitmentId || !missionId) {
      goBack();
      return;
    }

    fetchRequirement();
  }, []);


setMission(response?.data);

console.log(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

c: console.log 는 의도해서 남겨두신 부분일까요?

Copy link
Author

Choose a reason for hiding this comment

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

빠뜨린 부분 캐치해주셔서 감사합니다 😊


console.log(response);
} catch (error) {
alert((error as AxiosError).response?.data.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

c: AxiosError인지 등은 UI Component가 신경쓰지 않아도 되는 부분이지 않을까요? api 레이어에 있는 fetchMissionRequirements 에서 필요한 검증은 마치고, 컴포넌트에서는 에러 메지시 노출, 페이지 이동 등 UI와 관련된 부분만 신경쓰도록 하는 것이 좋다고 생각합니다~!

Copy link
Author

Choose a reason for hiding this comment

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

아, 맞아요! UI Component가 오류 형식을 받아서 메시지만 보여주는 게 가장 이상적인 구조라는 데 동의해요~

지원플랫폼에서는 대부분 서버에서 받아온 오류 메시지를 그대로 표시하기보다는, 오류가 발생하면 클라이언트에서 정의한 메시지 상수를 보여주는 구조로 되어 있더라고요. 서버에서 받아온 오류 메시지를 직접 표현하는 처리는 몇 군데 없는 편이에요.

그런데 현재로서는 API 레이어에서 catch하는 구조가 아직 완벽하게 갖춰지지 않은 것 같아요! 이 부분은 앞으로의 리팩터링 작업에서 꼭 개선해 나가야 할 방향이 아닐까 싶어요~! 그때까지는 현재의 구조를 유지하면서 점진적으로 개선해 나가는 게 좋을 것 같아요 😊

}, []);

useEffect(() => {
highlighter.highlightAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

c: highlight 대상은 어떻게 찾나요 ?-?

라이브러리 문서에 아래와 같이 안내되고 있긴 하군요 🤔

This will find and highlight code inside

 tags; it tries to detect the language automatically. If automatic detection does not work for you, you can specify the language in the class attribute:

이후에 어떤 식으로 적용되는지 코드만으로 이해하기 어려울 수 있을 것 같아, 이 방식이 유지된다면 주석을 같이 남겨두어도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

주석에 간단히 남겨둘게요~

);
};

export default MissionView;
Copy link
Contributor

Choose a reason for hiding this comment

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

c: page 컴포넌트들은 Storybook도 같이 넣어두고 있었는데요. 이후 테스트 보강을 같이 해두면 좋을 것 같습니다.

Copy link
Author

@woowahan-cron woowahan-cron left a comment

Choose a reason for hiding this comment

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

바쁜 일정에도 코드 리뷰 남겨주셔서 감사합니다!
추후 작업이 필요한 내용은 별도의 이슈에서 진행해보겠습니다~!
감사합니다

const { token } = useTokenContext();

const goBack = () => {
navigate(-1);
Copy link
Author

Choose a reason for hiding this comment

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

이 의도에 대해서는 구체적으로 논의된 적이 없어서요!
일단 '뒤로 가기'기능으로 구동하더라도 큰 문제가 되지 않으니 적용해 보고 적절치 않으면 다음 이슈에서 진행해보는 것이 좋을 것 같아요 😁

try {
if (!recruitmentId || !missionId) {
goBack();
return;
Copy link
Author

Choose a reason for hiding this comment

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

goBack() 같은 내비게이션 동작은 분리해내는 게 더 명확할 것 같아 코멘트만 남겨둡니다.
3주 뒤에 코드를 다시 보니 명확해 보이지 않는다는 피드백에 동감합니다 😅

처음 의도는 뒤에 이어지는 useEffect에 동일한 방어 로직(if (!recruitmentId || !missionId))을 작성해야 하는데 중복되는 코드를 작성해야 하나 생각했었거든요. 겹치더라도 분리해 내는 것이 적절해 보여서 반영해 두겠습니다!

 useEffect(() => {
    if (!recruitmentId || !missionId) {
      goBack();
      return;
    }

    fetchRequirement();
  }, []);


setMission(response?.data);

console.log(response);
Copy link
Author

Choose a reason for hiding this comment

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

빠뜨린 부분 캐치해주셔서 감사합니다 😊

@@ -5,6 +5,8 @@
"dependencies": {
"axios": "^0.21.1",
"classnames": "^2.3.1",
"github-markdown-css": "^5.6.1",
Copy link
Author

Choose a reason for hiding this comment

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

라이브러리를 선택한 배경에 관해 궁금해하시는 것 같습니다!
서버에서는 DB에 마크다운 형식으로 가지고 있지만, 클라이언트에 전달할 때는 html로 변환해서 던져주게 됩니다.
그래서 이 상황에 적합한 라이브러리는 github-markdown-css 라고 판단했습니다 🙂

@@ -30,7 +30,7 @@ export type FetchMyMissionJudgmentResponseData = Judgment;
export type FetchMyMissionsResponseData = Mission[];

export type FetchAssignmentRequest = RequestWithToken<{
recruitmentId: number;
recruitmentId: string;
Copy link
Author

Choose a reason for hiding this comment

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

요청을 먼저 날려서 되는지를 확인하려고 타입을 잠시 바꿔둔 것이었는데 나중에 후속 작업을 누락했던 것으로 추정하고 있습니다.
다시 변경해 두었습니다! 그런데 코드 곳곳에 string으로 선언된 부분들도 있어서 나중에 리팩터링할 때 형식을 통일하는 작업이 필요해 보입니다

className={buttonStyles["assignment-button"]}
disabled={
missionItem.status === MISSION_STATUS.ENDED ||
missionItem.status === MISSION_STATUS.UNSUBMITTABLE
Copy link
Author

Choose a reason for hiding this comment

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

좋은 방법 알려주셔서 감사해요 ㅋㅋ
제가 제출 상태에 대한 이해도가 부족했어서요!

missionItem.status === MISSION_STATUS.ENDED ||
missionItem.status === MISSION_STATUS.UNSUBMITTABLE
}
onClick={() => {
Copy link
Author

Choose a reason for hiding this comment

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

반영해둘게요~

@@ -8,7 +8,7 @@ type MissionProps = {
recruitmentId: string;
};

const missionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
export const getMissionLabel = (submitted: boolean, missionStatus: Mission["status"]) => {
Copy link
Author

Choose a reason for hiding this comment

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

  • 미션 제출 상태에 따라 적합한 레이블 텍스트를 뽑아 내는 함수인데 이것이 훅이 가지고 있는 함수라기보다는, 유틸 함수로 빼야 하는 것 아닌가? 하는 고민에서 시작되었습니다.
  • 필요하다면, 별도 이슈에서 작업할게요~

}, []);

useEffect(() => {
highlighter.highlightAll();
Copy link
Author

Choose a reason for hiding this comment

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

주석에 간단히 남겨둘게요~


console.log(response);
} catch (error) {
alert((error as AxiosError).response?.data.message);
Copy link
Author

Choose a reason for hiding this comment

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

아, 맞아요! UI Component가 오류 형식을 받아서 메시지만 보여주는 게 가장 이상적인 구조라는 데 동의해요~

지원플랫폼에서는 대부분 서버에서 받아온 오류 메시지를 그대로 표시하기보다는, 오류가 발생하면 클라이언트에서 정의한 메시지 상수를 보여주는 구조로 되어 있더라고요. 서버에서 받아온 오류 메시지를 직접 표현하는 처리는 몇 군데 없는 편이에요.

그런데 현재로서는 API 레이어에서 catch하는 구조가 아직 완벽하게 갖춰지지 않은 것 같아요! 이 부분은 앞으로의 리팩터링 작업에서 꼭 개선해 나가야 할 방향이 아닐까 싶어요~! 그때까지는 현재의 구조를 유지하면서 점진적으로 개선해 나가는 게 좋을 것 같아요 😊

@woowapark
Copy link
Contributor

+) 과제 보기 페이지 컴포넌트의 이름이 적절한지 여부에 대한 질문을 주셨던 부분이 있었군요
현재로도 문제는 없다고 생각하고요 :) 리뷰 코멘트로 질문만 남겨보자면 해당 데이터를 불러오기 위해 사용하는 함수에서는 MissionRequirements라는 용어가 쓰이던데 이게 공통의 용어라면 페이지 컴포넌트의 이름도 그대로 MissionRequirements로 써도 괜찮겠다는 의견만 남겨봄미당 🙏

Copy link

@wmakerjun wmakerjun left a comment

Choose a reason for hiding this comment

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

크론~! 과제 보기 페이지까지 구현하느라 너무 고생하셨습니다 👏🏻
제가 코드 리뷰를 너무 늦게 진행해버렸네요 😭
간단한 코멘트 중심으로 남겼고, MissionView에서만 개선 제안을 남겨봤습니다. 리팩터링의 측면이기 때문에 이번 PR에서 진행 안해주셔도 괜찮고, 다음에 진행해봐도 좋을것 같아요~!

이번 작업도 너무 고생 많으셨습니다!

navigate(
generatePath(PATH.MISSION_VIEW, {
recruitmentId,
missionId: String(mission.id),

Choose a reason for hiding this comment

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

a: missionId를 문자열로 변환하고 있는데, 이 부분이 꼭 필요한 것일까요~?(제가 코드를 잘 몰라서 하는 질문일수도 있습니다~!) 만약 API나 다른 코드에서 이 변환이 반드시 필요하다면 실수를 줄이기 위한 코드를 추가해 볼 수도 있을것 같아 보여요~!

Copy link
Author

Choose a reason for hiding this comment

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

추후 타입 정리도 리팩터링 작업 진행해보겠습니다~!

@@ -4,6 +4,7 @@ export const PATH = {
SIGN_UP: "/sign-up",
MY_APPLICATION: "/applications/me",
ASSIGNMENT: "/assignment/:status",
MISSION_VIEW: "/assignment/:recruitmentId/mission/:missionId",

Choose a reason for hiding this comment

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

a: 혹시 assignment/:assignmentId 또는 recruitment/:recruitmentId 가아닌, /assignment/:recruitmentId 이런 계층 구조여야하는 이유가 있는 것일까요~? 🤔 다소 헷갈리는 느낌이 있는데, 아마 이미 도메인 관련해서 논의 및 의사결정이 있었을 것 같아요. 과제 안에 여러개의 미션이 있는 의미일것 같긴한데 참고용 질문으로만 남겨봅니다~!

Copy link
Author

Choose a reason for hiding this comment

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

QA 진행 기간 중에 확인해 보고 코멘트 달아두겠습니다~!

);
};

const fetchRequirement = async () => {

Choose a reason for hiding this comment

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

c: MissionView라는 이름은 과제 보기 페이지를 렌더링하는 역할을 주로 수행할 것으로 예상되는데 컴포넌트 안에서 데이터 불러오기, 네비게이션, 상태 관리 등 다양한 로직을 포함하고 있는 것 같아요~!

데이터를 불러오는 로직이나 네비게이션 로직을 별도의 커스텀 훅으로 분리해보는건 어떨까요~? 재사용할 계획이 없다면, 분리하지 않을 수 있겠으나, 현재는 컴포넌트 네임의 역할을 확인하는데 한 곳에서 많은 코드를 봐야하는것 같아 제안드려 봅니다~!

Copy link
Author

@woowahan-cron woowahan-cron Sep 3, 2024

Choose a reason for hiding this comment

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

공원의 리뷰와 비슷한 관점에서 말씀해 주신 것 같아요!

이 부분은 앞으로의 리팩터링 작업에서 꼭 개선해 나가야 할 방향이 아닐까 싶어요~! 그때까지는 현재의 구조를 유지하면서 점진적으로 개선해 보겠습니다~!

@woowahan-pjs woowahan-pjs changed the title feat: Placing task view button feat(apply): implement the mission view page Sep 12, 2024
@woowahan-pjs woowahan-pjs merged commit 7ca413d into develop Sep 12, 2024
@woowahan-pjs woowahan-pjs deleted the feature/mission-description-view branch September 12, 2024 01:30
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.

4 participants