-
Notifications
You must be signed in to change notification settings - Fork 1
[4팀] 프론트엔드 코드리뷰용 PR #22
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
Conversation
|
안녕하세요, 프론트엔드 멘토 최다인입니다. |
src/entities/d-day/store.ts
Outdated
| }; | ||
|
|
||
| // Zustand 스토어 생성 | ||
| export const useDdayStore = create<DdayStore>(set => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zustand 사용 관련해서 서면 피드백 요청 주셨네요.
우선 보통 전역으로 관리할 store는 store 디렉토리에 모아서 관리하는것이 일반적입니다.
entities 디렉토리는 api, request 관련한 디렉토리로 보이는데, store는 별도로 관리해주면 좋을 듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전역 store를 사용하실 때에는, 서비스 전반에 걸쳐 많은 곳에서 참조되는 데이터인지 생각해보면 좋습니다.
현재 useDdayStore는 DdayDateBottomSheet, NewSchedule 정도에서 참조하는 것으로 보이네요. (NewScedule에서는 왜 참조하는지 궁금합니다 👀)
전역 상태로 관리되는 일반적인 데이터는, 로그인 정보, 유저 정보, 앱의 테마 등이 있습니다. 현재 dday 데이터가 전역에서 참조되어야할 데이터인지 검토하면 좋을 듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요, 다인님 😃
귀한 시간 내셔서 코드 리뷰 남겨주셔서 진심으로 감사합니다.
현재 NewSchedule 페이지는 두 가지 타입의 일정을 하나의 화면에서 생성할 수 있도록 구성되어 있는데요, 이 부분은 추후에 두 개의 페이지로 분리하면서 코드 리팩토링도 함께 진행할 계획입니다.
지금은 전역 상태를 통해 일정을 구성하는 값들을 관리하고 있으며, NewSchedule 페이지의 확인 버튼을 눌렀을 때 해당 값을 API 요청에 활용하는 구조입니다. 그래서 NewSchedule에서 dday 전역 상태를 가져와 API 요청의 매개변수로 사용하고 있는 상황입니다.
저도 이 부분에서 전역 상태를 사용하는 게 살짝 어색하다는 느낌이 들어서 고민이 되었는데요, 말씀 주신 것처럼 전역 상태 대신 각 페이지에서 필요한 데이터를 useState로 관리하고, DateBottomSheet, TimeBottomSheet 같은 UI 컴포넌트에는 setState를 props로 넘겨서 상태를 업데이트하는 방식이 더 적절할지 고민 중입니다.
혹시 이 부분에 대해 다인님의 의견이나 경험이 있다면 조언 부탁드릴 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
병현님 안녕하세요🙂
리팩토링을 진행하실 계획이시군요. 좋은 경험이라고 생각합니다.
먼저 두 가지 타입의 일정에 대해서, 입력하는 필드만 다를 경우, 페이지를 분리하면 일정 생성과 관련하여 중복되는 로직이 생길 수 있음에 유의해야합니다. 구체적인 개발 요구사항에 따라 다르겠지만, 중복되는 부분은 공용화하고 분리되는 부분을 최소화한다면 도움이 될 것입니다.
일정 생성 상태관리에 관하여 답변 드리면,
- 생성중인 일정 상태를 전역에서 사용하면 좋은 케이스는, 해당 페이지를 벗어난, 다른 페이지에서 해당 상태를 참조하는 경우를 생각해볼 수있습니다. (ex 작성 중 미리보기 페이지로 이동하는 등)
- 따라서 지금은 페이지의 state로 관리하는 것이 좋을듯 하며, props로 넘겨주는 방식이 적합해보입니다.
- 개발을 진행하면서 props drilling이 심해질 경우에 context api 등의 props drilling 해결 방법 적용도 검토해볼 수 있습니다.
src/entities/d-day/store.ts
Outdated
| import { create } from "zustand"; | ||
| import { DdayStore, InitialDday } from "./type"; | ||
|
|
||
| // 초기 상태 | ||
| const initialState: InitialDday = { | ||
| id: null, | ||
| title: "", | ||
| emoji: "CALENDAR", | ||
| date: "", | ||
| }; | ||
|
|
||
| // Zustand 스토어 생성 | ||
| export const useDdayStore = create<DdayStore>(set => ({ | ||
| dday: initialState, | ||
| setTitle: title => set(state => ({ dday: { ...state.dday, title } })), | ||
| setEmoji: emoji => set(state => ({ dday: { ...state.dday, emoji } })), | ||
| setDate: date => set(state => ({ dday: { ...state.dday, date } })), | ||
| reset: () => set({ dday: initialState }), | ||
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zustand 자체는 맞게 사용하고 계셔서 다른 추가 피드백을 드려보면,
속성 하나하나를 업데이트 하기보다는 dday를 통째로 업데이트하는 메서드가 있다면 유지보수에 좋을 것 같습니다. 현재 구조는 dday의 속성이 하나 추가될 때 마다 액션이 추가되어야하기 때문입니다.
아래와 같이 작성해 볼 수 있습니다. (IDE 환경에서 작성된 것이 아니라, 문법적 오류가 있을 수 있습니다.)
import { create } from 'zustand';
interface InitialDday {
id: null | number;
title: string;
emoji: string;
date: string;
}
interface DdayState extends InitialDday {
updateDday: (newDday: Partial<InitialDday>) => void;
resetDday: () => void;
}
const initialState: InitialDday = {
id: null,
title: '',
emoji: 'CALENDAR',
date: '',
};
export const useDdayStore = create<DdayState>((set) => ({
...initialState,
updateDday: (newDday) => set((state) => ({ ...state, ...newDday })),
resetDday: () => set(() => ({ ...initialState })),
}));| resetOnboarding: () => void; | ||
| } | ||
|
|
||
| export const useOnboardingAlarmStore = create<OnboardingAlarmState>(set => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앞서 드린 DdayStore와 피드백 내용 동일합니다.
우선은 개발 일정이 우선이고, 시간이 되실 때 피드백을 차근차근 적용해보시길 바랍니다.
혹시 feature base 폴더 구조를 가져가시려던거면,
onboarding/store.ts 로도 충분합니다.
fix: 디데이 없을 때 뜨는 UI 조건 수정 및 디자인 수정1
refactor: 주석 추가
rename: 페이지 분리
refactor: 바텀시트 대신 페이지로 편지 내용 볼 수 있게 변경
remove: 편지 내용 볼 수 있는 바텀시트 컴포넌트 삭제
refactor: carousel 컴포넌트 모듈로 관리되게 변경
fix: 편지 카드 이미지 없을 때 UI 안 보이게 변경
fix: 댓글 생성 후 form 리셋
design: 뒤로가기 버튼 스타일 수정
fix: 댓글 짤리는 오류 해결
design: 상태 선택되면 버튼 색깔 변경되게 수정
src/pages/calendar/NewSchedule.tsx
Outdated
| const [newScheduleState, setNewScheduleState] = useState<InitialSchedule>(initialState); | ||
|
|
||
| // API 훅 | ||
| const { mutate } = useCreateScheduleMutation(newScheduleState); | ||
|
|
||
| const { selectedMonth, selectedDay } = useSelectedDateStore(); | ||
| // 유효성 검사 | ||
| const isFormValid = useMemo(() => { | ||
| return newScheduleState.title.trim() !== "" && newScheduleState.startDate !== "" && newScheduleState.endDate !== ""; | ||
| }, [newScheduleState.title, newScheduleState.startDate, newScheduleState.endDate]); | ||
|
|
||
| const { mutate } = useCreateScheduleMutation(schedule, reset, selectedMonth, selectedDay); | ||
| // 이벤트 핸들러 | ||
| const handleTitleChange = (title: string) => { | ||
| setNewScheduleState(prev => ({ | ||
| ...prev, | ||
| title, | ||
| })); | ||
| }; | ||
|
|
||
| const handleStartDateChange = (date: string) => { | ||
| setNewScheduleState(prev => ({ | ||
| ...prev, | ||
| startDate: date, | ||
| })); | ||
| }; | ||
|
|
||
| const handleEndDateChange = (date: string) => { | ||
| setNewScheduleState(prev => ({ | ||
| ...prev, | ||
| endDate: date, | ||
| })); | ||
| }; | ||
|
|
||
| const handleAllDayToggle = () => { | ||
| setNewScheduleState(prev => ({ | ||
| ...prev, | ||
| isAllDay: !prev.isAllDay, | ||
| })); | ||
| }; | ||
|
|
||
| const handleFatigueChange = (fatigue: Fatigue) => { | ||
| setNewScheduleState(prev => ({ | ||
| ...prev, | ||
| fatigue, | ||
| })); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@da-in 다인님 안녕하세요!
이전에 멘토링해주신 내용을 바탕으로 코드를 수정해보았는데요, 시간 괜찮으실 때 코드 리뷰 부탁드립니다!
기존에는 zustand를 이용해 일정을 생성하는 데이터를 전역 상태로 관리하고 있었는데, 이번에는 useState를 이용해서 컴포넌트 내부에서 입력값들을 관리하도록 바꿔보았습니다.
그 과정에서 각각의 입력값을 관리하기 위한 이벤트 핸들러가 총 5개 생겼는데, 지금처럼 페이지 컴포넌트 내부에서 직접 선언하고 관리하는 방식이 괜찮은지 궁금합니다.
추가로, 현재는 props 드릴링이 거의 없지만, 이 5개의 이벤트 핸들러를 하나의 Context로 묶어서 관리하는 것도 고려해보고 있습니다. 이런 경우에도 Context를 사용하는 것이 좋은 선택일지, 아니면 커스텀 훅을 만들어 관리하는 방향이 더 좋을지 고민이 됩니다
피드백 주시면 감사하겠습니다! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로, 지금 이 페이지는 일정 생성 페이지인데요.
이제 일정 수정 페이지도 새로 만들어야 하는 상황입니다.
이럴 때 하나의 페이지에서 '생성'과 '수정'을 분기처리해서 각각에 맞는 훅을 연결해 작업을 수행하는 게 나을지,
아니면 '수정 전용 페이지'를 따로 만들어 기능을 분리하는 게 나을지 고민되고 있습니다.
혹시 어떤 방식이 더 명확하고 유지보수하기 좋을지 조언해주실 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 전역 상태에서 지역 상태로 전환한 부분 매우 좋습니다.
- 컴포넌트 내에 핸들러가 있는것은 자연스럽습니다. 여러 핸들러를 하나로 통일할 수 있는 방법을 생각해보시면 좋을 듯 합니다. 프로퍼티 별로 핸들러를 업데이트 하는것이 아니라,
newScheduleState를 업데이트하는 하나의 핸들러로 충분할 것 같습니다. 아래 이해를 위한 코드 첨부합니다.
const handleInputChange = useCallback(
(name: keyof InitialSchedule, value: any) => {
setNewScheduleState((prev) => ({
...prev,
[name]: value,
}));
},
[]
);
...
<TitleInput value={newScheduleState.title} onTitleChange={(title) => handleInputChange("title", title)} />- Context API는 props drilling이 심해졌을 때 도입고려하시면 되겠습니다. 핸들러가 많은 이슈는 2의 방향으로 해결하시면 좋을 듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로, 지금 이 페이지는 일정 생성 페이지인데요. 이제 일정 수정 페이지도 새로 만들어야 하는 상황입니다.
이럴 때 하나의 페이지에서 '생성'과 '수정'을 분기처리해서 각각에 맞는 훅을 연결해 작업을 수행하는 게 나을지, 아니면 '수정 전용 페이지'를 따로 만들어 기능을 분리하는 게 나을지 고민되고 있습니다.
혹시 어떤 방식이 더 명확하고 유지보수하기 좋을지 조언해주실 수 있을까요?
- 일정 생성과 수정 기능에서 얼마나 코드가 중복되는지를 확인해야합니다. UI가 거의 동일하고 API등의 로직만 다르다면 분기 처리를 하는것이 좋을 것입니다. 하지만 UI가 전혀 다르다면 페이지는 분리하는 것이 오히려 좋을 것이고, 서로 다른 페이지에서 공통된 로직을 공용화하는 방향을 생각해볼 수 있습니다. 요지는 코드 중복을 줄이고 공용화하는 것입니다. 중복된 코드가 존재하면 수정을 해야할 때 동일한 작업을 여러번 해야하기 때문입니다.🙂
refactor: infoCard UI 요소 구조 변경
feat: 편지 카드에 모달 연결
feat: 편지 삭제 API 연결
refactor: 편지 생성 훅 변경
refactor: 주석 추가
feat: 편지 수정 API 함수 및 훅 작성
feat: 편지 작성 컴포넌트 수정 후 다시 연결
fix: 필요없는 매개변수 정리
refactor: 필요 없는 코드 제거 및 디데이 카드에서 이모지 제거
remove: 필요없는 코드 제거
fix: 푸시알림 오류 해결
revert: 푸시알림 롤백
fix: 화면 비율 수정
style: 탑 마진 설정
design: 패딩 수정
[수정사항] 핸드폰 상단바 영역 앱 차지
fix: 푸시알림 코드 수정
revert: 푸시알람 롤백
[수정사항] 상단바 여백 추가
fix: 토큰 없을 시 리턴문 추가
remove: 서비스워커 등록 코드 제거1
revert: 롤백
remove: sw 코드 주석처리
fix: sw 일부만 주석처리
revert: 롤백
chore: gitignore 수정
No description provided.