Conversation
iamidlek
left a comment
iamidlek
left a comment
There was a problem hiding this comment.
고생하셨습니다.
전체적으로 비슷한 구성이 이어져
일부 코멘트가 중복될 것 같은 부분은 건너뛰었습니다.
총평
전체적으로 기능은 잘 구현해 주신 것 같습니다.
캘린더 날짜 계산이나 주차 계산이나 여타 컴포넌트를
직접 잘 로직을 생각해 구현해 주신 것 같아 좋았습니다.
다만 아쉬운 점이 조금 많이 보이는 것 같습니다.
- 폴더 구조 layout, page, component가 섞여 있는 것 같습니다.
- 비로그인 시 추가 안되는 부분 등 error 발생 시 아무 경고나 가이드가 없어서 아쉽습니다.
예외 처리 부분이 아쉽습니다. - styled 용 컴포넌트가 잘 나누어지지 않아 재사용 없이 모든 파일 마다 동일한 역할인데
선언되는 것 같습니다(예: Container) - 사용이 안되는 부분이 코드로 남아있는 부분이 아쉽습니다.
- 색상이나 font size 등 전부 하드 코딩 되어있어서 theme으로 관리하는 것이 어떨까 싶습니다.
| <Route path="/detail" element={<Detail />}></Route> | ||
| <Route path="/" element={<Home />}></Route> | ||
| {/* 상단에 위치하는 라우트들의 규칙을 모두 확인, 일치하는 라우트가 없는경우 처리 */} | ||
| <Route path="*" element={<NotFound />} /> |
There was a problem hiding this comment.
NotFound를 맨 마지막 줄에 두는 것이 좋을 것 같습니다.
기능은 정상 동작은 하는데
/graph까지 안가고 *에서 걸릴 것 같은 구조로 보입니다.
| @@ -0,0 +1,49 @@ | |||
| // 소비 기록을 추가하거나 수정할 때 서버 요청 데이터 | |||
There was a problem hiding this comment.
파일명과 선언된 타입의 연관성이 없는 것 같습니다.
안 쓰는 부분도 있는 것 같고 사용되는 쪽에서 선언하거나
해당 파일 혹은 디렉토리 내에서 나누어 관리되면 좋을 것 같습니다.
| import Detail from '@/components/detail/Detail'; | ||
| import { BrowserRouter, Routes, Route } from 'react-router-dom'; | ||
| import Home from '@/components/Home/Home'; | ||
| import '@/App.css' |
There was a problem hiding this comment.
; 가 안 붙어 있는데 prettier 설정이 다들 잘 된 것일까요?
There was a problem hiding this comment.
prettier 설정을 다같이 논의하고 넘어간 부분이긴한데 세미콜론 누락이 되었는지 모르고 넘어갔습니다.
| sortSummariesFunc() | ||
| }, [summaries]) | ||
|
|
||
| const handlePeriod = (value: string) => { |
|
|
||
| return ( | ||
| <div> | ||
| <StyledCanvas ref = {chartRef} /> |
There was a problem hiding this comment.
ref={chartRef} 뿐만 아니라
다른 파일에도 비슷하게 prettier 적용이 안된 부분이 보입니다.
| const StyledSelect = styled.div ` | ||
| display: flex; | ||
| justify-content: center; | ||
| flex-align: justify; |
| if (chartRef) { | ||
| const ctx = chartRef.getContext('2d'); | ||
| if (ctx) { | ||
| const weeksInMonth = getWeeksInMonth(selectedYear, selectedMonth); | ||
| if (weeksInMonth[index]) { | ||
| const week = weeksInMonth[index]; | ||
| const weekTitle = index === 0 ? '첫째 주' : `${getOrdinalWeek(index)} 주`; | ||
| let existingChart = Chart.getChart(chartRef); | ||
| if (existingChart) { | ||
| existingChart.data.labels = week.map((date) => date.split('-')[2]); | ||
| existingChart.data.datasets[0].data = week.map((date) => { | ||
| const expenseItem = expenseData.find((item) => item._id === date); | ||
| return expenseItem ? expenseItem.totalAmount : 0; | ||
| }); | ||
| existingChart.update(); | ||
| } else { | ||
| existingChart = new Chart(ctx, { | ||
| type: 'bar', |
There was a problem hiding this comment.
너무 깊어지는 것 같습니다. 어떤 처리를 하는지
나누어 명확하게 할 필요가 있어 보입니다.
| const months = []; | ||
| for (let i = 1; i <= 12; i++) { | ||
| months.push( | ||
| <option key={i} value={i}> | ||
| {i}월 | ||
| </option> | ||
| ); | ||
| } |
There was a problem hiding this comment.
항상 동일한 값을 가질 것 같은 처리들이
컴포넌트 안에 있는 것 같습니다.
There was a problem hiding this comment.
@iamidlek 항상 동일한 값을 가질 것 같은 처리들이라고 하면 이 부분을 상태관리나 useMemo를 이용하는게 낫다는 말씀맞으신가요?
| <AmountInput | ||
| ref={inputRef} | ||
| dir="rtl" | ||
| type="text" |
There was a problem hiding this comment.
문자 입력에 대한 방지 처리를 handleAmountChange 따로 하고 있는데
type 을 number로 주면 간단히 해결 될 것 같습니다.
There was a problem hiding this comment.
이 부분은 개인적으로 생각했던게 tpye을 number로 할 경우 input 필드 내부에 생기는 컨트롤러가 보기 불편해서 이렇게 처리하였는데, input type을 number로 변경해서 컨트롤러를 제거할 수 있나요?
| import { API_URL, HEADERS, userId } from '@/lib/api/Base'; | ||
|
|
||
| // 소비 기록 작성 | ||
| export const createdExpense = async (data: ExpenseData) => { |
There was a problem hiding this comment.
createdExpense 를 사용하는 곳(AddModal)에서 try, catch로 해당 함수를
감싸고 있지 않은데 해당 함수의 catch 맨 밑의
throw error는 어디서 잡아서 처리를 해주는 것일까요?
There was a problem hiding this comment.
제가 그 부분은 미처 생각하지않고 동작하니깐 바로 넘어가버렸습니다.
const handleSubmit = async () => {
try {
// type이 expense일 때 amount가 음수로 서버에 데이터에 전달되게
let formAmount = amount;
if (type === 'expense') {
formAmount = -amount;
}
// category에 소비 태그 값이 있고, paymentMethod값이 있을 때 카테고리에 추가
let category = tag;
if (paymentMethod) {
category += `, ${paymentMethod}`;
}
const data = {
amount: formAmount,
category: category,
date: new Date(selectDate).toString(),
};
await createdExpense(data);
if (onItemUpdated) {
onItemUpdated();
}
close();
} catch (error) {
console.error(error);
}
};
try catch를 사용해서 에러 처리를 할 수 있도록 수정하도록 하겠습니다.
There was a problem hiding this comment.
조금 더 이야기를 드리면 createdExpense 에서 throw를 하시는 이유가
있어야 할 것 같습니다.
단순히api 요청 쪽에서 console을 찍고 메세지를 throw 하여
컴포넌트 내에서 동작할 때 다시 console을 찍는 것이 의미가 없어 보입니다.
catch와 throw를 용도에 맞게 사용하면 더욱 좋지 않을까 하는 의견입니다.
There was a problem hiding this comment.
제가 코로나로 인해 몸상태가 안좋아서 늦게나마 댓글을 달게되었습니다 ㅠㅠ 신경써주셨는데 죄송합니다.
에러처리에 대해서 통상적으로 api통신하는 컴포넌트에서 에러처리를 하고 넘겨야하는지, 아니면 api통신하는 컴포넌트에서는 말 그대로 통신만하고, 사용하는 컴포넌트에서 에러처리를 하는 방법 중에 어떠한 방법을 통상적으로 많이 사용하는지 궁금합니다.
throw 날렸던건 개인적으로 진행하면서 통신할 때 에러처리하면서 고정적으로 써보니 따로 동작 상에 문제가 없어서 그렇게 굳어졌던거 같습니다.
3 participants
7조 - 💵 This-is-money (이게머니)
배포사이트
이게머니
Repository
This-is-money repo
프로젝트 팀원
유희태
남기훈
이정환
정승원
일자 및 태그 별 지출 내역
깃 관리, 프로젝트 팀장
입/지출 내역 생성 모달 작업
내역 수정/삭제 모달 작업
내역 상세 정보 모달 작업
detail 차트 생성 작업
detail 리스트 추가 작업
기간+카테고리 필터 기능 작업
주간 분석 기능 작업
기술 스택
Development
Config
Deployment
Environment
Cowork Tools
Commit Convention
feat: 새로운 기능 추가fix: 버그 수정docs: 문서 수정style: 코드 포맷팅, 세미콜론 누락, 코드 변경이 없는 경우refactor: 코드 리펙토링test: 테스트 코드, 리펙토링 테스트 코드 추가chore: 빌드 업무 수정, 패키지 매니저 수정Pull Request Convention
프로젝트 구성
|