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: 일정 만들기 추가 구현 #55

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open

Conversation

Choi-Jinwook
Copy link
Contributor

@Choi-Jinwook Choi-Jinwook commented Nov 30, 2023

구현사항

  • 썸네일 선택 후 이미지 표시 로직 추가
  • progress 이동에 따른 사이드바 펼침 상태 핸들링
  • 템플릿, 작성 중 일정에 대해 msw 적용
  • 기본정보 임시저장 msw 적용
  • 장소 선택 모달 추가

TEST

pages/_app.tsx Outdated Show resolved Hide resolved
Copy link
Member

@pepperdad pepperdad left a comment

Choose a reason for hiding this comment

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

확인했습니다! 고생하셨습니다 ~

items: "schedules/items",
};

export const LOCATION = {
Copy link
Member

@pepperdad pepperdad Dec 10, 2023

Choose a reason for hiding this comment

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

👍

const BASE_URL = "http://localhost:3000/api";

export const basicInfoHandlers = [
http.post(`${BASE_URL}/${CREATE_SCHEDULE_PATH.temporary}`, ({ request }) => {
Copy link
Member

Choose a reason for hiding this comment

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

mock API 에서 post의 경우에 이렇게 하면 되겠군요!? 저도 적용해보겠습니다 감사합니다 ㅎ

callType: "first" | "second";
}

const CitySelector = ({ callType }: CitySelectorProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

[Lv3] 일정 찾기 페이지에서 이 모달을 사용하려고 하니까, 이 모달 컴포넌트 내부에서 recoil의 answer 값들을 변경하는 것 같아서 지금 사용하지 못하고 있는 상황입니다!
값 변경 로직 (여기서 handleLocation 함수)을 이 컴포넌트에서 관리하는 것이 아니라 prop로 받아서 변경하는 로직은 어떨까요!?
그럼 공통 모달로서 공용 컴포넌트로 사용할 수 있어서 더 좋을 것 같고, 굳이 여기서 한 번 더 recoil을 사용해서 한 번 더 부르지 않고 관리할 수 있어서 좋을 것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 네네 확인했습니다! handleLocation 함수를 LocationModal에서 혹은 따로 완료버튼 컴포넌트 만들어서 그 안에 작성하도록 하겠습니다
혹시 지금 형태 자체는 필터에 적용하기에 괜찮을까요?

Copy link
Member

Choose a reason for hiding this comment

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

넵 그 부분만 수정되면 괜찮을 것 같아요!

Copy link
Member

@pepperdad pepperdad left a comment

Choose a reason for hiding this comment

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

코멘트 추가로 작성했습니다!
확인해주시면 감사하겠습니다 ㅎㅎ

Copy link
Contributor

@NacreousCloud NacreousCloud left a comment

Choose a reason for hiding this comment

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

확인했습니다! 코멘트 확인 부탁드려용

try {
const res = await postBasicInfo(answer);

if (res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[Lv1] 이 조건문은 res가 undefined나 null로도 넘어올수있다는건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분 msw 구성할 때 연결 확인하려고 만들어놨다가 그대로 넘어온 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

res 넘어오는 데이터 타입 확인 후 필요없다면 조건문 제거하는것이 좋아보입니다!

<div className="w-[628px] border border-[#E0E0E0] rounded-[5px] px-[8px] mb-[20px] focus-within:border-[#4285F4]">
<div
id="태그"
className="w-[628px] border border-[#E0E0E0] rounded-[5px] px-[8px] mb-[20px] focus-within:border-[#4285F4]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className="w-[628px] border border-[#E0E0E0] rounded-[5px] px-[8px] mb-[20px] focus-within:border-[#4285F4]"
className="w-[628px] border border-[#E0E0E0] rounded-md px-2 mb-5 focus-within:border-[#4285F4]"

[Lv2] E0E0E0 4285F4 둘다 자주 사용되고있습니다! 따로 분리해주세요!

alt="camera"
width={30}
height={30}
className="rounded-[4px]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className="rounded-[4px]"
className="rounded-s"

@@ -14,16 +14,15 @@ const SideBarMenuBox = ({
handleToggle,
}: SideBarMenuBoxProps) => {
return (
<div className="w-full h-[63px] -tracking-[0.5px] px-[27px] py-[20px]">
<div
className="w-full h-[63px] -tracking-[0.5px] px-[27px] py-[20px] cursor-pointer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className="w-full h-[63px] -tracking-[0.5px] px-[27px] py-[20px] cursor-pointer"
className="w-full h-16 -tracking-[0.5px] px-6 py-5 cursor-pointer"

}
};

export const setButtonDisabled = (
Copy link
Contributor

Choose a reason for hiding this comment

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

[Lv2] 해당 함수는 "버튼을 Disabled시키는 목적" 보다는 "버튼의 상태가 disabled인지 체크" 하는 함수라고 판단됩니다! 함수의 명칭은 확장성을 좀 넓게 잡고 가는게 좋을것같아요
추가로, return 값이 boolean이라면 접두사로 is~ 를 붙이는게 좋습니다!
너무 추상적으로 해도 export할때 애로사항이 많아지기에 getIsDisabled, 혹은 아예 유효성 체크를 목적으로 getButtonValidation 같은 명칭으로 교체하는걸 추천드려요!

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.

3 participants