Feature/fix chakra progress theme error #115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SeungJL
changed the title
seongbin9786
reviewed
May 9, 2024
|
|
||
| interface IProgressBar { | ||
| value: number; | ||
| colorScheme?: "mintTheme"; |
There was a problem hiding this comment.
이 prop은 당장은 상수여서 엄청 필요한 prop은 아닌 것 같습니다 :)
혹시 어떤 의도로 prop을 만드셨는지 궁금합니다 :)
| import { faQuestionCircle } from "@fortawesome/pro-light-svg-icons"; | ||
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
| import { useSession } from "next-auth/react"; | ||
| import { useEffect, useState } from "react"; | ||
| import styled from "styled-components"; | ||
| import ProgressBar from "../../../components/atoms/ProgressBar"; |
There was a problem hiding this comment.
지금 당장 적용할 수는 없겠지만 tsconfig에 절대 경로 설정을 하게 되면 import 경로가 조금 더 간결해질 수도 있을 것 같습니다 :)
| colorScheme="mintTheme" | ||
| hasStripe | ||
| hasStripe={true} |
There was a problem hiding this comment.
이 부분은 convention으로 정할 수도 있을 것 같은 부분인 것 같습니다 :)
혹시 변수로 정해지는 boolean prop이 아닐 때도 true/false를 명시하는 걸 선호하실까요? :)
| const giftUsers = await GiftModel.find({ giftId: id }).select( | ||
| "-_id -createdAt -updatedAt -__v", | ||
| ); | ||
| if (!giftUsers) { |
There was a problem hiding this comment.
별 건 아니지만 여기 if 문 위에 개행이 들어가면 조금 더 깔끔할 수도 있을 것 같습니다 :)
- 아래 소스 파일을 봤을 땐 if 문 개행에 대한 컨벤션이 따로 없는 것 같습니다 :)
- 어떤 컨벤션을 정하거나 지금처럼 질서 부여하지 않고 갈 수도 있을 것 같습니다 :)
| @@ -11,7 +11,7 @@ import { useAdminStudyRecordQuery } from "../hooks/admin/quries"; | |||
| import { useImageUploadMutation } from "../hooks/image/mutations"; | |||
| import { studyDateStatusState } from "../recoils/studyRecoils"; | |||
| function Test() { | |||
| const { data } = useAdminStudyRecordQuery(dayjs("2024-04-01"), dayjs("2024-04-07"), null, "인천"); | |||
| const { data } = useAdminStudyRecordQuery(dayjs("2024-04-16"), dayjs("2024-04-30"), null, "인천"); | |||
There was a problem hiding this comment.
단순 궁금증입니다 :)
- 혹시 이 test 페이지의 의도가 테스트 용도라면 이 소스 파일도 버전 관리가 되어야 하는 것인지 궁금합니다 :)
| .sort("createdAt") | ||
| .select("-_id -createdAt -updatedAt -__v"); | ||
|
|
||
| res.status(200).json({ |
There was a problem hiding this comment.
사소한 부분입니다 :)
- 200 등의 HttpStatusCode를 enum이나 리터럴 선언을 해서 가져다 쓰면 의도가 조금 더 명확하게 표현될 수도 있을 것 같습니다.
- 다만 http status는 너무 익숙해서 숫자가 더 간결할 수도 있을 것 같습니다 :)
- 만약 200이 아니라 201, 204, 301, 302, 304, 400, 401, 403, 404, 500, 502, 503 이렇게 섞어쓰게 된다면 상수화를 하면 위의 이유 때문에 조금 더 좋을 것 같습니다 :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #114