-
Notifications
You must be signed in to change notification settings - Fork 10
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
[2주차] 권혜인 미션 제출합니다. #3
base: master
Are you sure you want to change the base?
Conversation
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주차 과제 너무 고생하셨습니다!!🥰🥰🥰
따로 상태 관리 라이브러리를 쓸 수 없어서 props drilling때문에 많이 고생하셨죠?😭😭😭
그런데 배포된 링크가 권한 문제로 열리지 않는 것 같은데, 한번 확인해주실 수 있나요?
...react.configs.recommended.rules, | ||
...react.configs['jsx-runtime'].rules, | ||
...reactHooks.configs.recommended.rules, | ||
'react/jsx-no-target-blank': 'off', |
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/jsx-no-target-blank
규칙을 끌 필요는 없을 것 같은데, 원래 사용하시던 규칙들을 가져오신건가요?!
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-refresh/only-export-components': [ | ||
'warn', | ||
{ allowConstantExport: true }, | ||
], |
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-refresh
라는 eslint 규칙이 있다는 걸 배웠습니다..! 그런데 이쪽도 이번 프로젝트에서는 설정할 필요 없는 것 같아요!
추가로, 지금처럼 상수들을 export할 때 jsx 파일이 아니라 color.js
같이 js 파일에서 export하신다면 문제가 발생하지 않으므로 삭제하셔도 될 것 같아요!!
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.
네! 감사합니다 😄
@@ -0,0 +1,43 @@ | |||
{ |
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.
추가로 Vite를 이용해서 프로젝트를 잘 세팅해주신 것 같아요!!👍👍
그런데 혹시 따로 Vite를 사용하신 이유가 있을까요?
또, 프로젝트 안에 따로 week2-ceos
폴더를 따로 만들고 프로젝트를 세팅하셨는데 root 폴더에 바로 세팅하지 않으신 이유가 있을까용?! 지금대로면 root에서 npm start
를 하면 빈 페이지가 실행되고, 원하는 페이지를 열려면 week2-ceos
로 이동해서 npm run dev
로 실행시켜야 할 것 같은데, 처음 프로젝트를 보는 사람이면 실행법을 헷갈릴 수 있다고 생각했습니다! 만약 week2-ceos
파일들만 사용하면 root에 있는 src 등의 파일들은 다 지워도 될 것 같아요 ;)
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.
아 일단 vite 를 사용한 이유는,
일단 yarn + vite 를 쓰는게 습관이 된 이유도 있고, cra가 vite보다 상대적으로 느려서요!
사실 이번 과제와 같이 작은 플젝은 별로 상관은 없지만, 그래도 이왕이면 빠르고 최신거?를 사용하자!는 마음으로 진행했습니다!
그리구 원래 과제를 할 때 저만의 폴더를 만들고 시작하는게 맘이 편하더라구요,,ㅎㅎ
return ( | ||
<ThemeProvider theme={theme}> | ||
<MainPage /> | ||
<GlobalStyle /> |
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.
globla style을 잘 지정해주신 점 너무 좋아요👍👍
} | ||
|
||
const DayWrapper = styled.span` | ||
${rowFlex} |
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.
rowFlex
를 따로 만들어 두는 방법은 생각 못했는데, 정말 좋은 것 같아요!
const filtered = todoList.filter((list) => list.day === clickedDay); | ||
if (filtered.length > 0) { | ||
setFilteredTodoList(filtered[0].todos); | ||
} else { | ||
setFilteredTodoList([]); | ||
} |
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.
이 부분도 위에서 언급드린 것처럼 find
를 이용하실 수 있을 것 같아요!
export default function TodosHeader(props) { | ||
const { todoList, clickedDay } = props; | ||
const [filterTodoList, setFilteredTodoList] = useState([]); | ||
const [isDoneList, setIsDoneList] = useState([]); |
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.
아래 코드를 읽어 보니 isDoneList
는 완료된 할 일의 갯수를 나타내는 것 같네요! 그런데 초기화는 빈 배열로 되고 있어서 코드를 읽는 데 혼란을 줄 수 있을 것 같아요. 이 부분 네이밍+초기값을 수정하시면 좋을 것 같아요!!
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.
하하맞아요,,, 진짜 네이밍 제일 어려운것ㅠㅠㅠ
{filterTodoList.length > 0 && | ||
filterTodoList.map((content) => { |
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.
filterTodoList
의 길이를 체크하고 map
을 사용하는 방법이 좋네용!
저는 이런 경우에 filterTodoList?.map((content) => ...
처럼 쓰거나, filterTodoList
가 항상 배열임이 보장되면 그냥 filterTodoList.map((content) => ...
처럼도 쓰는데 괜찮은 것 같아요! 혜인님은 어떻게 생각하시나요?
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.
저는 뭔가 에러를 조기에 잡을 수 없다는 생각이 들어서 길이 같이 바로 보이는 걸로 체크하고 코딩하는게 습관화된 것 같아요,,!!!
@@ -0,0 +1,116 @@ | |||
import { create } from "zustand"; |
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 쓰신 부분이 이부분이군요..! 결국 사용하지 않게 된 것 같아서 이번 코드리뷰에서는 건너뛸게용..! 다음 코드 리뷰에서 뵈어요🙇🙇
:root { | ||
font-family: Inter, system-ui, Avenir, Helvetica, Arial, sans-serif; | ||
line-height: 1.5; | ||
font-weight: 400; | ||
|
||
color-scheme: light dark; | ||
color: rgba(255, 255, 255, 0.87); | ||
background-color: #242424; | ||
|
||
font-synthesis: none; | ||
text-rendering: optimizeLegibility; | ||
-webkit-font-smoothing: antialiased; | ||
-moz-osx-font-smoothing: grayscale; | ||
} |
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.
더 많은 코드리뷰를 남기고 싶었으나 혜인님 배포 링크가 열리지 않아서,,,코드만 보고 유추하기엔 아직 미숙하여 제대로 된 코드리뷰가 안 된 것 같습니다...! 링크 접속이 가능하게 된다면 다시 코드를 살펴보겠습니다. 그게 아니더라도 정말 배울 점이 많았던 것 같습니다! 고생많으셨습니다 😁
week2-ceos/.eslintrc.js
Outdated
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.
eslint 설정관련해서 공부가 필요했는데 좋은 래퍼런스가 될 것 같습니다 감사합니다!
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.
스타일을 유동적으로 바꿀 수 있게 설정한 것이 디자인 구조를 구성하는 데 있어 더 편리할 것 같습니다. 다양한 방법 배워갑니다!
import { rowFlex } from "@styles/commonStyle"; | ||
|
||
export default function TodoInput(props) { | ||
const { handleInput, inputRef } = 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.
혹시 이 부분에서 setTodoText도 같이 내렸는데 따로 안 받는 이유가 있을까요?!
<Wrapper> | ||
<LabelTextWrapper> | ||
<CheckBoxLabel> | ||
<CheckBox checked={isDone} onChange={handleCheck} $boxColor={boxColor} type="checkbox" /> |
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.
$로 스타일 Props를 내려줘서 구분과 동시에 사용할 수 있다는 것을 알게 되었습니다! 감사합니다~
outline: 4px auto -webkit-focus-ring-color; | ||
} | ||
|
||
@media (prefers-color-scheme: light) { |
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.
이런식으로 적용할 수도 있군요! @media에 대해 더 공부해봐야겠습니다
✨ 구현 기능 명세
구현배포링크
🌟 미션 목표
🌠 Key Question
블로그에 정리해두었습니다
💎 필수 요건
🧩 PR Point
💜 Modal
할 일을 입력하는 부분을 추가하는 버튼을 클릭하면, 모달을 띄울 수 있도록 하였습니당,,,
useState
로 Modal의 open, closed를 제어하도록 하였습니다.💚 Input
Input value 를
useRef
훅을 사용하여, 제어할 수 있도록 코드를 구성했습니다.💛 customHook
💙 colorBox
사용자가 클릭한 색깔을 이용해서 checkbox의 border + backgroundColor 나타나도록
🧡 주간 별 날짜들이 나타나는 컴포넌트
🩷 styled-component
styled-component
에서 globalStyle, theme을 만들어 두어서 필요할 때 불러와 사용했습니다.가장 많이 쓰는
flex
관련 부분은 commonStyle 에 미리 저장해 두어 불러와 사용했습니다.🔥 Issue Log
블로그에 같이 정리해두었습니다
🥹 느낀 점
일단,,,, 처음에 과제 요건을 제대로 안 보고 zustand로 전역 상태 관리를 해버렸지 뭐에요,,ㅎㅎㅎ 이걸 제출 직전에 발견해서 부랴부랴 customhook으로 바꾸고 props drilling으로 바꾸었습니다. 시간이 나면 context api를 이용해서 리팩토링해보려고 합니다!!
저번에 너무 시간을 못 써서 이번에는 최대한 커밋도 나누고, 디자인도 예쁘게 하려고 노력했습니다! 디자인,,은 사실 제가 자주 쓰는 어플인 TimeBlocks의 할일 부분에서 따왔습니다..! (거의 따라했다,,?도 무방,,ㅎㅎㅎ) react,,, 다시 정말 열심히 공부해야 할 거 같고 별거 아닌 부분에서 에러가 생각보다 많이 터져서 이런 부분도 잘 정리해두어야 할 것 같아요:-)