Feat: 일정 채우기 페이지 구현 #52
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ); | ||
| }; | ||
|
|
||
| export default TemplatePreview; |
There was a problem hiding this comment.
코드 리뷰를 위해 주어진 코드 패치를 살펴보면 다음과 같습니다:
import문에서@create-schedule/components와@shared/hook모듈을 가져오고 있습니다.TemplatePreview함수 컴포넌트가 정의되어 있습니다.useModal훅에서closeModal을 사용하고 있습니다.currentDate변수에 현재 날짜로 초기화된Date객체가 할당되고 있습니다. 하지만// TODO: 날짜 받아오기라는 주석이 추가되어 있으므로, 실제로는 외부에서 날짜를 받아와야할 것으로 보입니다.- JSX를 통해 UI를 렌더링하고 있습니다.
<CurrentDate>컴포넌트에currentDateprop으로currentDate값을 전달하고 있습니다.<TimeTable>컴포넌트에callType="template"prop으로 값을 전달하고 있습니다. 그러나TODO: initialData 받아오기라는 주석이 추가되어 있어 초기 데이터를 받아와야할 것으로 보입니다.- 확인 버튼을 클릭하면
closeModal함수가 호출됩니다.
버그 또는 개선 제안:
TODO주석에 따라서 날짜와 초기 데이터를 얻는 부분을 구현해야 합니다.- UI 요소의 스타일 및 레이아웃을 검토하여 개선할 수 있는 부분이 있는지 확인해야 합니다.
- 코드에서 주석을 줄이고 가독성을 높이는 것이 좋습니다. 주석은 필요한 경우에만 사용하는 것이 좋습니다.
이러한 점들을 고려하여 코드를 수정하면 될 것 같습니다.
|
|
||
| interface MakeScheduleButtonProps extends HTMLAttributes<HTMLButtonElement> { | ||
| interface MakeScheduleButtonProps | ||
| extends ButtonHTMLAttributes<HTMLButtonElement> { | ||
| value: string; | ||
| buttonStyle: string; | ||
| } |
There was a problem hiding this comment.
주어진 코드 패치에 대해 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험 요소 및 개선 제안을 환영합니다:
react에서HTMLAttributes를 가져오는 대신ButtonHTMLAttributes를 가져오는 것으로 수정되었습니다.MakeScheduleButtonProps인터페이스가 확장됩니다.HTMLButtonElement에 대한 속성을 가집니다.value와buttonStyle속성이 추가되었습니다.
개선 사항:
buttonStyle을 문자열 대신 열거형(enum)으로 정의하는 것이 타입 안정성에 도움이 될 수 있습니다.- 다른 속성(예:
onClick)도 필요한 경우 해당 속성을 인터페이스에 추가하는 것이 좋습니다.
이 외에는 주요한 문제점은 없어 보입니다.
| isClicked={isClicked} | ||
| onClick={onClick} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| export default Template; |
There was a problem hiding this comment.
위의 코드 패치는 Template 컴포넌트와 관련되어 있습니다. 컴포넌트에 대한 간단한 코드 리뷰를 도와드리겠습니다.
버그 주의 사항:
isClicked및onClick은TemplateProps인터페이스의 속성으로 지정되었습니다. 그러나 실제로는isClicked라는 속성이 존재하지 않습니다. 이 문제를 해결하려면ScheduleCard컴포넌트에isClicked속성을 추가해야 합니다.
개선 제안:
isClicked에서 불린 값 대신에,"template"및"custom"키를 가진 객체가 전달되도록 하는 것이 좋을 수 있습니다.onClick에 있는 키 타입 정의를 개선하여 타입 안정성을 확보할 수 있습니다.
아래는 개선된 코드 예시입니다:
import { templateContent } from "@create-schedule/constants";
import { ScheduleCard } from "@shared/components";
interface TemplateProps {
activeType: "template" | "custom";
onClick: (key: "template" | "custom") => void;
}
const Template = ({ activeType, onClick }: TemplateProps) => {
return (
<ScheduleCard
content={templateContent}
callType="template"
isClicked={{ template: activeType === "template", custom: activeType === "custom" }}
onClick={onClick}
/>
);
};
export default Template;위의 개선된 코드는 activeType을 이용하여 isClicked 객체를 전달합니다. 또한 onClick 함수에 대한 키 타입 정의를 개선하여 사용자에게 더 명확하고 안정적인 API를 제공합니다.
| {...isClicked} | ||
| clickedContent={clickedContent} | ||
| onClick={handleClickCard} | ||
| /> | ||
| ) : ( | ||
| <Continue /> | ||
| )} |
There was a problem hiding this comment.
주어진 코드 패치를 간략히 검토해보겠습니다:
-
useModal이라는 훅을 새로 추가하여 임포트하고 있습니다. 해당 훅의 내용과 사용 방법을 확인해야 합니다. -
onClickprops가 추가되었습니다.handleClickCard콜백 함수를 클릭 이벤트 처리에 사용하고,openModal함수를 호출합니다. -
handleClickCard와openModal함수가callType및 해당 조건에 따라 다른 동작을 수행하므로, 해당 동작을 검토해봐야 합니다. -
onClick함수명이onClick으로 변경되었습니다. -
<TemplateButton>컴포넌트로 전달되는 props에서isClicked객체를 전달받게 되었습니다. -
마지막 부분에서
<TemplateButton>의 렌더링 조건과 함께onClick콜백도 확인해야 합니다.
| </div> | ||
| </div> | ||
| </> | ||
| )} | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
해당 코드는 비교적 작은 수정으로 구성되어 있습니다. 몇 가지 개선 제안을 드리면 다음과 같습니다:
-
import문 정리: 현재 코드에 추가된 컴포넌트를 import하고 있으므로, 필요한 import 문은 모두 주석 처리된 상태입니다. 실제 사용하는 컴포넌트들을 import 해주세요. -
jsx 태그 정렬: JSX 요소의 열린 태그와 닫힌 태그를 맞춰주기 위해 각 줄의 들여쓰기를 확인하여 일관된 스타일로 작성하세요.
-
클래스 이름 변경:
PageContent컴포넌트의 첫 번째<div>요소에 클래스 이름이 추가되었습니다. 이를 수정하여 일관된 규칙을 유지하세요. -
Title컴포넌트 props:Title컴포넌트의title및subTitleProps 값은 상수에서 가져오는 것으로 보입니다. 해당 상수 값을 표시하기 전에 정의된 값인지 확인하고, 변수 또는 문자열 값으로 변경하여 사용하세요. -
번역 문구 확인:
subTitle프롭스에 있는 "명란마요"라는 부분이 번역되어야 할 내용인지 확인하세요. -
인라인 스타일 개선:
TimeTableContainer컴포넌트와DragnDropContainer를 감싸는<div>요소에 인라인 스타일이 사용되었습니다. 가능하다면 CSS 파일로 해당 스타일을 이동시켜 관리하기 쉽도록 변경하세요. -
리팩토링: 중복 코드가 있거나 코드 품질 및 가독성을 개선할 수 있는 부분은 없어 보입니다. 그러나 코드 기반과 프로젝트의 컨벤션을 고려하여 필요한 경우 추가적인 리팩토링을 수행하세요.
위 제안사항을 참고하여 코드를 수정하시면 됩니다.
| ); | ||
| }; | ||
|
|
||
| export default TimeTableContainer; |
There was a problem hiding this comment.
코드 리뷰를 해드리겠습니다.
- 코드 전체적으로는 깔끔하고 가독성이 좋습니다.
import DateContainer from "./DateContainer";와import TimeTable from "./TimeTable";가 필요한 모듈을 임포트하는 것 같습니다. 해당 모듈들이 존재하고 올바르게 동작하는지 확인해야 합니다.TimeTableContainerProps인터페이스는 현재 빈 객체로 정의되어 있습니다. 필요한 프롭스(prop)나 프로퍼티(property)를 추가하여 다른 부분과의 상호 작용이 명확하도록 만들 수 있습니다.min-w-[362px],max-w-[362px]클래스 속성은 특정 너비를 지정하는 듯합니다. 이에 대해서는 레이아웃 요구사항이나 스타일링 관련 규칙을 확인해야 합니다.<div className="w-full border border-[#ACBEFF] rounded-[5px] bg-[#FFF9FC]">에서border와background-color값들이 직접 하드코딩된 것을 볼 수 있습니다. 이런 값들을 프롭스로 받아서 설정할 수 있다면 컴포넌트의 재사용성과 유연성을 높일 수 있습니다.
전반적으로 위 코드는 버그나 크리티컬한 결함을 보이지 않으며, 개선할 사항들은 사소한 부분입니다. 잘 작성된 코드입니다.
| ) | ||
| return; | ||
|
|
||
| setAppliedItem((prev) => { |
There was a problem hiding this comment.
[Lv2] 해당함수 중복 로직 개선이 가능할 것같습니다! array return 방식을 통일시켜보는 방식으로 해보시는건 어떨까요?
There was a problem hiding this comment.
혹시 감이 안오는데 어떤 방식을 말씀하시는지 알려주실 수 있으실까요..?
There was a problem hiding this comment.
이런.. 확인이 늦었네요..
selectedItem을 넣는건 조건 여부와 상관없이 동일하기 때문에 해당 조건을 통일시키는 의미였습니다!
setAppliedItem((prev) => {
if (selectedItem) {
return [
...(prev ? prev : []),
{
...selectedItem,
startTime: { hour: startHour, minute: startMin },
endTime: { hour: endHour, minute: endMin },
},
];
}
return null;
});| { imageSrc: "/images/samples/category_etc.svg", title: "기타" }, | ||
| ]; | ||
|
|
||
| export const category: CategoryItem[] = [ |
There was a problem hiding this comment.
[Lv2] Const는 대문자로 작성해주세요!
There was a problem hiding this comment.
msw 데이터 부분으로 뺄 예정이라 대충 때려박았습니다..! feat/makeplan 브랜치에서는 msw세팅 완료해서 해당 부분 삭제되어 있습니다!
| className="w-full h-full text-[#333333] outline-0" | ||
| onChange={({ target: { value } }) => handleInput("category", value)} | ||
| > | ||
| <option>기타</option> |
There was a problem hiding this comment.
[Lv2] shared/types 에 CategoryTags에서 전체 만 빼면 되는 option들입니다! 가져와서 사용하시면 될거같아요
There was a problem hiding this comment.
[Lv2] useEffect의 의존성인 time이 바뀐다면 해당 컴포넌트 전체가 다시 렌더링되는것과 다를게 없기 때문에, useEffect를 빼고 handleDuration을 setDuration을 하는게 아닌, 결과값만 리턴시켜서 렌더링 시키는게 나아보입니다!
| case "customScheduleSelector": | ||
| return <CustomScheduleSelector />; | ||
| case "templatePreview": | ||
| return <TemplatePreview />; | ||
| default: | ||
| return <></>; | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 도와드리겠습니다.
- import 문
- 현재 코드에는 추가된 모듈(import)이 있는데, 해당 모듈들은 잘 추가되었는지 확인이 필요합니다.
- CustomScheduleSelector, ScheduleTimeSelector, TemplatePreview 모듈이 임포트되어야 합니다.
- switch 문
- switch 문을 보면 case 별로 다른 컴포넌트를 반환하고 있습니다.
- 각각의 case에 대해 어떤 동작을 수행하는지 확인이 필요합니다.
- 반환하는 컴포넌트가 해당 모듈에서 제대로 가져오는지 확인해야 합니다.
- ModalContentProps 타입
- ModalContentProps 타입에 대한 설명이 없어서 해당 타입이 어떤 속성과 값을 가지는지 파악할 수 없습니다.
- ModalContentProps의 정의를 확인해보고, 사용하는 곳에서 유효한 값들이 전달되는지 확인해야 합니다.
| @@ -89,4 +89,7 @@ export type ModalContentId = | |||
| | "thumbnailSelector" | |||
| | "calendarSelector_start" | |||
| | "calendarSelector_end" | |||
| | "scheduleTimeSelector" | |||
| | "customScheduleSelector" | |||
| | "templatePreview" | |||
| | "RecruitManage"; | |||
There was a problem hiding this comment.
주어진 코드 패치에 대해 간단하게 코드 리뷰를 도와드리겠습니다. 버그 위험 및 개선 제안을 환영합니다.
이 코드 패치에서는 다음과 같은 변경 사항이 있습니다:
- ModalContentId 유형에 새로운 값을 추가했습니다:
- "scheduleTimeSelector"
- "customScheduleSelector"
- "templatePreview"
개선 제안:
- 코드에 문제점은 보이지 않습니다. 추가한 모달 컨텐츠 ID들이 적절한 형식으로 선언되어 있는 것 같습니다.
- 그러나 코드 리뷰만으로는 프로덕션 환경에서 발생할 수 있는 모든 잠재적인 버그를 확인하기는 어렵습니다. 이 코드 패치를 더 깊이 검토하고 실행 시나리오에서의 테스트를 통해 버그를 찾을 수 있습니다.
- 또한, 해당 코드 패치와 상호작용하는 다른 부분도 고려해야 합니다. 다른 파일 내용이나 종속성에 의한 영향을 평가하는 것이 중요합니다.
더 자세한 코드 검토나 개선 사항을 위해서는 코드의 전체 내용을 확인해야 합니다.
변경사항
Test
TODO