-
Notifications
You must be signed in to change notification settings - Fork 0
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
5조 과제 제출 (김가은, 김경원, 김준희, 정재현) #2
base: main
Are you sure you want to change the base?
Conversation
import loadable from "@loadable/component"; | ||
import Loading from "@/components/common/Loading"; | ||
|
||
const MainLayout = loadable(() => import("@/layout/MainLayout"), { |
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.
loadable
라이브러리를 사용하신 것을 칭찬드립니다. 코드 스플리팅을 통해 라우터마다 번들로 나누어 두고, 필요할 때 동적으로 import하는 방식을 사용하신 것 같은데 라이브러리를 사용한 의도가 이게 맞을까요?🤔
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.
2차 토이프로젝트까지 완벽하게 수행해주셔서 감사합니다! 장기 개인 휴가로 이제서야 코드리뷰를 작성하게 되어서 죄송합니다.🙇🏻♂️ 리팩토링이나 라이브러리 관련 질문들이 있는데, 질문들에 대해서 개인적으로 답변을 달아보시면서 왜 이렇게 구현했는지 한 번 더 생각해보는 시간이 되기를 바랍니다!
// 7. 소비 기록 달력 호출 | ||
export const getCalendar = async (year: number, month: number, userId: string) => { | ||
try { | ||
const res = await baseApi.get(`/expenses/calendar?year=${year}&month=${month}&userId=${userId}`); |
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.
axios의 경우, get에 대한 URL parameter들을 3번째 인자의 객체 형태로 전달하여 사용할 수 있습니다.
await baseApi.get(`/expenses/calendar`, {
params: {
year: year,
month: month,
userId: userId,
}
}
);
의 방식으로 사용하면 가독성이 좋아질 것 같아서 조언드립니다.
@@ -0,0 +1,36 @@ | |||
import styled from "styled-components"; | |||
import { Icon } from "@iconify/react"; |
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.
react가 아닌 외부 라이브러리인 iconify
를 사용하셨네요. 아이콘의 경우, 상용에서도 필요할텐데 해당 라이브러리가 왜 devDependencies
에 존재하는 것인지 궁금합니다!
</StyledLink> | ||
</MainHeaderFirstRow> | ||
<MainHeaderSecondRow> | ||
<StyledLink to="/main/daily" $isActive={location.pathname === "/main/daily"}> |
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.
$isActive
와 같은 달러 표기 props는 경우, styled-components
라이브러리의 표기법으로, 스타일을 위한 props라고 이해하면 될까요?
const userId = useUserStore((state) => state.userId); | ||
const totalLists = useExpensesStore((state) => state.totalLists); | ||
const setTotalLists = useExpensesStore((state) => state.setTotalLists); | ||
const currentYear = useTimeStore((state) => state.currentYear); |
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
를 사용하였으나, 실제로 current값은 호출 시점마다 new Date()
를 통해서 새롭게 만들어진 값을 반환하고 있는데요. store를 사용하는 목적이 기존의 데이터를 저장하기 위함인데, 호출시점마다 new Date()
를 통해서 새롭게 만들어진 값을 사용한다면 저장의 의미가 사라지게될 것 같은데 어떻게 생각하시는지요??!
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를 사용하기 보다는 현재 시점의 년도, 월, 시간을 반환하는 하나의 함수의 형태로 구현하는 것이 올바르다고 생각합니다!
<StyledLink to="/main/daily" $isActive={location.pathname === "/main/daily"}> | ||
<div>일일</div> | ||
</StyledLink> | ||
<StyledLink to="/main/weekly" $isActive={location.pathname === "/main/weekly"}> |
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.
URL 주소에서 /moayo/#/main/weekly
와 같이 중간에 #
이 들어가는데, 혹시 어떠한 의미를 가지는지 알 수 있을까요? URL의 경우, 웹상에서 특정 리소스를 가르키고 있으므로, 리소스의 의미를 담아야합니다. 현재 main
페이지에서 weekly
주간 정보를 보기위함이라고 알 수 있겠으나, #
은 어떠한 의미를 지니는지 궁금합니다.
) { | ||
setConsumption( | ||
totalLists | ||
.filter((item) => item.date.includes(currentYearMonth)) |
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.
totalLists
.filter((item) => item.date.includes(currentYearMonth))
.filter((item) => item.amount > 0)
.reduce((sum: number, item: search) => {
return sum + item.amount;
}, 0)
의 로직이 중복되고 있는데요. 자세히 보시면 filter의 조건 함수만 변경되고 있음을 볼 수 있습니다.
function getNewTotalList(testFunc) {
return totalLists
.filter(testFunc)
.reduce((sum: number, item: search) => {
return sum + item.amount;
}, 0)
}
의 방식으로 조건 함수를 인자로 받아 새로운 값을 반환하는 함수로 중복을 제거할 수 있을 것 같습니다:)
.toLocaleString()} | ||
</MonthlyConsume> | ||
<MonthlyIncome> | ||
{lists |
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.
여기도 위와 동일하게 조건 함수를 인자로 받아 새로운 값을 반환하는 함수로 중복을 제거할 수 있을 것 같습니다:)
function getAmount(list, testFunction) {
return list
.filter(testFunction)
.reduce((acc, cur) => acc + cur.amount, 0)
.toLocaleString()
}
import { useUserStore } from "@/store/useUserStore"; | ||
import { ConsumptionTags, IncomeTags } from "@/constants/tags"; | ||
|
||
const Add = () => { |
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.
추가를 진행할 때, 단순히 저장버튼이 disabled
되는 것보다는 어떠한 이유로 되지 않는 것인지 확인할 수 있도록 알럿이나 모달을 띄우면 좋을 것 같아요. 예를 들어 금액을 비워둔 상태에서 저장하기를 누르면 금액이 비워져있다거나, 이런식으로 에러핸들링을 명확하게 분할하여 사용자가 사용하기 편리하게끔 가이드를 주는게 좋을 것 같아요! 다음 프로젝트에서는 꼭 해보시길 추천드립니다.
|
||
const CalendarFormFullCalendar = () => { | ||
const [events, setEvents] = useState<EventObject[]>([]); | ||
const [year, setYear] = useState<number>(new Date().getFullYear()); |
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.
기존의 현재 시점의 year, month, day를 반환하는 함수를 구현해두고 통일된 함수를 사용하는 것이 좋을 듯 합니다. 각 컴포넌트 별로 new Date()
를 통해서 날짜 객체를 생성하여 사용하고 있는데, 이를 하나의 날짜 관련 함수(포맷팅 포함)을 구현하여 모든 컴포넌트에서 해당 함수를 사용하게 되면, 컴포넌트 별로 모두 동일한 값을 가지도록 보장할 수 있습니다:)
createDayList(); | ||
}, [monthList, currentYear, currentMonth]); | ||
|
||
const createDayList = () => { |
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.
createDayList가 Daily
와 Monthly
컴포넌트에서 중복되고 있습니다. 해당 중복을 제거할 수 있도록 dayList를 생성하는 메서드를 구현하여 재사용하도록 하면 좋을 것 같아요!.
💰 모아요 (MOAYO)
React, TypeScript, Rest API를 활용한 간편 가계부 프로젝트입니다.
프로젝트 소개
개발자 소개
(일일, 주간, 월간, 전체)
(일일, 주간, 월간, 전체)
Chart 페이지
사용기술 및 개발환경
Development
Config
Deployment
Environment
Cowork Tools
프로젝트 테스트
clone project
go to project
$ cd moayo
install npm
start project
프로젝트 상세 기능
Main
일일 (/main/daily)
주간 (/main/weekly)
월간 (/main/monthly)
Add
Edit
Calendar
Chart
Search
Account
기타 기능
프로젝트 회고
iOS 환경에서 주간 리스트가 나오지 않는 에러사항이 있습니다.
크롬이나 다른 데스크탑 브라우저와 안드로이드OS에서는 문제 없이 출력되는 것을 확인했습니다.
해당 오류에 대해 알고 계신 분이 있다면 메일 부탁드립니다..!