-
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주차] 이예지 미션 제출합니다. #1
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.
안녕하세요 예지님! 프론트 운영진 안채연입니다
이번주 과제도 역시나 너무 깔끔하게 해주셨네요!
로컬스토리지 사용부터 할 일 수정기능까지...! 너무 수고많으셨습니다
이번주 발표이신걸로 알고 있는데 발표가 기대가 되네요 ㅎㅎ
너무 수고많으셨고 발표 화이팅입니닷!
padding-left: 10px; | ||
padding: 0px 20px 15px 20px; |
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.
아래 padding
값을 모두 지정해주었으니까 padding-left: 10px;
는 없어도 되겠네요!
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.
허거덩... 이걸 놓쳤네요ㅠㅠㅠ 감사합니다!!!
const Section = styled.div` | ||
flex: 0.5; | ||
border-top: 2px solid rgb(177, 171, 171); | ||
overflow: auto; |
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.
overflow: auto
이용해서 스크롤 잘 주셨네요! 필수는 아니지만 혹시몰라 스크롤바 커스텀 참고자료 놓고 갑니다! 스크롤바까지 커스텀하면 더 디테일이 살아날 것 같아요 ㅎㅎ
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.
저번 주 과제에서 스크롤 바 커스텀이 가능한 걸 알았는데!! 미처 적용까지 해보진 못 했어요😿 다음 번엔 자료 참고해서 꼭 커스텀 해보겠습니당!!
const submitTodo = (e) => { | ||
e.preventDefault(); | ||
|
||
if (value.trim()) { |
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.
저번 주 과제 코드 리뷰때 채연님이 알려주셔서 적용했습니다!! :)
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.
혹시 이부분은 입력이 있을 경우를 처리하면
인덴테이션이 깊어지니까
입력이 비어있을 경우 먼저 처리해서 return으로 끊어줘도 좋을 것 같아요 😆
(실제로 카카오나 네이버 등 클린 코드 작성을 위해 인덴테이션을 최소화하라고 권장하고 있다고 해요! 😃 )
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.
오~!! 확실히 그런 경우로 작성하면 코드 양이 훨씬 줄어들겠네요!! 좋은 아이디어 감사합니당!!😆
#done-title { | ||
text-decoration: line-through; | ||
color: gray; | ||
} |
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.
종종 id
을 사용하는게 보이는데, 혹시 styled-component
를 사용하고 있음에도 불구하고 id
를 사용하시는 이유가 있을까요?
styled-component
를 사용할때 className
이나 id
를 함께 사용하는건 권장되지 않는다고 합니다! 이미 styled-component
를 사용하고 있는 만큼 className/id
대신에 컴포넌트로 관리해주면 좋을 것 같아요!
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.
헉 권장되지 않는군요!! id
를 사용했던 div
태그도 차라리 컴포넌트로 관리해야겠네요~!! 좋은 정보 감사합니다!!👏🏻👏🏻👏🏻
<div> | ||
<Enter> | ||
<form onSubmit={submitTodo}> | ||
<h2>Todo List ✔</h2> | ||
<input | ||
type="text" | ||
value={value} | ||
placeholder=" Enter your to do" | ||
onChange={handleChange} | ||
id="input-todo" | ||
/> | ||
<button type="submit" id="button"> | ||
➕ | ||
</button> | ||
</form> | ||
</Enter> | ||
</div> |
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.
여기 최상위에 div
태그로 한번 더 감싸준 이유가 있을까요?
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.
하이고... 무의식중에 div
태그를 막 써 버리는 습관이 고스란히 나타났네요...ㅜㅜ 알려주셔서 감사합니다!!
#input-todo { | ||
border: none; | ||
background-color: rgb(205, 222, 241); | ||
border-radius: 20px; | ||
height: 35px; | ||
font-size: 15px; | ||
width: 85%; | ||
} | ||
#button { | ||
border: none; | ||
background: none; | ||
font-size: 18px; | ||
width: 40px; | ||
} |
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.
마찬가지로 styled-component
를 이용해봅시다!
const Button = styled.button` | ||
border: none; | ||
background: none; | ||
color: rgb(104, 87, 134); | ||
font-size: 18px; | ||
`; |
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.
cursor: pointer
도 추가해주시면 마우스 동작이 더 명확하게 보일 것 같아요!
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.
우와웅 그렇군요!!! 디테일에 조금 더 신경 써야겠어요!!💪🏻
<div> | ||
{Todo.title} | ||
<Button onClick={() => toggleTodo(Todo.id)}>✔</Button> | ||
<Button onClick={() => deleteTodo(Todo.id)}>✖</Button> | ||
<Button onClick={() => setIsEditing(true)}>🛠</Button> | ||
</div> |
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.
여기도 마찬가지로 div
로 한번 더 감싸주신 이유가 궁금하네요!
const Button = styled.button` | ||
border: none; | ||
background: none; | ||
color: rgb(104, 87, 134); | ||
font-size: 18px; | ||
`; |
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.
헉 지금 보니까 그렇게 하는 게 더 효율적이겠네요!! 좋은 정보 감사합니당ㅎㅎ😻
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주차 과제 넘 수고하셨습니다 💪
{list.map((data) => | ||
data.completed ? ( | ||
<></> | ||
) : ( | ||
<Todo | ||
Todo={data} | ||
list={list} | ||
setList={setList} | ||
toggleTodo={toggleTodo} | ||
deleteTodo={deleteTodo} | ||
></Todo> |
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.
completed 된 todo 만 걸러내는 작업에 js의 filter 함수를 활용해보면 어떨까요?!
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.
앗 filter
함수를 쓰면 더 간략하게 쓸 수 있겠네요!! 감사합니다ㅎㅎ :)
const submitTodo = (e) => { | ||
e.preventDefault(); | ||
|
||
if (value.trim()) { |
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.
혹시 이부분은 입력이 있을 경우를 처리하면
인덴테이션이 깊어지니까
입력이 비어있을 경우 먼저 처리해서 return으로 끊어줘도 좋을 것 같아요 😆
(실제로 카카오나 네이버 등 클린 코드 작성을 위해 인덴테이션을 최소화하라고 권장하고 있다고 해요! 😃 )
let editedTodo = list.map((data) => { | ||
if (data.id === Todo.id) { | ||
data.title = editedTitle; | ||
} | ||
return data; | ||
}); |
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.
Map을 활용한 센스가 진짜 좋은 것 같아요 👍👍
@@ -0,0 +1,21 @@ | |||
import { createGlobalStyle } from 'styled-components'; |
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.
우와 ... styled-component 에서
전역으로 스타일을 지정하는 방법이 있는지 처음 알았네요..🥺 전 이걸 몰라서 App.css 에 항상 전역 스타일링 코드를 작성했었거든요..! 이렇게 배워갑니다 👍👍👍
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.
저도 이번 과제 하면서 새롭게 알게 됐어요!! 확실히 전역으로 관리하니까 편리하고 가독성도 높아지더라구요!! 제가 참고했던 자료입니다ㅎㅎ GlobbalStyle
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.
저도 이 부분은 처음 알았어요! 이번에 전역적인 스타일은 index.css에 작성했는데, styled-components
에서 createGlobalStyle
을 사용할 수 도 있다는 것 배워갑니다!
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.
안녕하세요 :)
코드 구경왔다가 리뷰 조금 놓고 갑니다!!!
이따 스터디때 봐요!!
const initialTodoData = localStorage.getItem('list') | ||
? JSON.parse(localStorage.getItem('list')) | ||
: []; |
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.
이부분은
const initialTodoData = localStorage.getItem('list') | |
? JSON.parse(localStorage.getItem('list')) | |
: []; | |
const initialTodoData = localStorage.getItem('list')??[]; |
혹은
const initialTodoData = localStorage.getItem('list') | |
? JSON.parse(localStorage.getItem('list')) | |
: []; | |
const initialTodoData = localStorage.getItem('list')||[]; |
와 같이 적어도 괜찮을 것 같아요!
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.
허걱스.... 저는 이 코드 쓰고 너무 뿌듯해 하고 있었는데... 이보다 더 간결하게 쓸 수 있었군요.... 충격적이지만 감탄... 감사합니다!!!
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.
근데 문기님이 제시해주신 코드에서, 첫번째 코드는 로컬스토리지에 아이템이 존재하는 경우(localStorage.getItem('list')가 true)에는 그 아이템들을 initialTodoData에 할당해줘야 하니까 [ ]가 아니라 JSON.parse(localStorage.getItem('list')가 되어야 할 것 같아요! 그래서 첫번째 코드랑 두번째 코드를 합쳐서 원래 예지님 코드처럼 localStorage.getItem('list') ? JSON.parse(localStorage.getItem('list')) : [ ]로 적는 게 더 간단하지 않을까 하는 저의 작은 의견,,, 그런데 문기님처럼 로지컬 연산자(&&, ||) 활용하는 거 너무 좋은 것 같아요!!
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.
앗 JSON.parse부분을 뺐네요 ㅜㅠ 연산자 사용해서 한줄로 굳이굳이 쓰자면 이렇게 쓸 수 있을 것 같습니다
const initialTodoData = localStorage.getItem('list') | |
? JSON.parse(localStorage.getItem('list')) | |
: []; | |
const initialTodoData = JSON.parse(localStorage.getItem('list')||"null")??[]; |
let countTodo = 0; | ||
list.map((data) => (data.completed ? data : countTodo++)); |
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.
요기는 filter사용해도 좋을 것 같아요
별 차이가 없기는 하지만, map은 약간 각각의 원소에 작업을 해주고 싶을 때
사용하고, filter은 특정한 data를 가진 원소를 걸러내고싶을 때
사용하는 것 같아서요!
큰차이는 없지만, filter와 map의 performance를 비교한 글을 놓고 갑니다..!
let countTodo = 0; | |
list.map((data) => (data.completed ? data : countTodo++)); | |
let countTodo = list.filter((data) => {data.completed}).length; |
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.
예지님 안녕하세요, 권가은입니다!!
이번 과제 하시느라 수고 많으셨습니다~!!
새롭게 추가하신 todo 수정 기능, 화면 구성요소들 최대한 컴포넌트 분리하신 사항 등 예지님 센스가 드러나신 코드 같아요. ㅎㅎ
??
연산자를 이용한 null, undefined 처리filter
와map
구분 사용- styled-components 사용시 id, class 사용 자제
등 제가 코멘트 달고자 했던 부분들에 이미 다른 분들이 코드리뷰를 너무 잘 해주셔서, 저는 간단한 코멘트만 추가해보았습니다!
다음 과제도 화이팅입니닷~!!
src/components/Form.js
Outdated
|
||
if (value.trim()) { | ||
getTodo(value); | ||
console.log(value); |
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.
console.log 은 일부러 남겨두신 걸까요?
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.
엇 이 부분 이전에 pending 된 코멘트라 같이 들어갔네요 😅😅
@@ -2,7 +2,7 @@ | |||
<html lang="en"> | |||
<head> | |||
<meta charset="utf-8" /> | |||
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" /> | |||
<link rel="icon" href="oyat_favicon.jpg" /> |
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.
이번 코드리뷰를 하며 알게됐는데, favicon 지정 시 png 등의 확장자 대신 ico 로 변환해 사용하는 게 더 좋다고 하더라구요!
링크 같이 첨부드립니닷
@@ -0,0 +1,21 @@ | |||
import { createGlobalStyle } from 'styled-components'; |
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.
저도 이 부분은 처음 알았어요! 이번에 전역적인 스타일은 index.css에 작성했는데, styled-components
에서 createGlobalStyle
을 사용할 수 도 있다는 것 배워갑니다!
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.
안녕하세요 예지님!
코드도 정말 깔끔하고, 투두리스트도 귀엽고, todo 수정할 수 있는 기능도 만들어주셔서 좋았어요...!
이번 과제 고생 많으셨고, 다음 과제도 같이 화이팅해봐요~!!💪💪
const initialTodoData = localStorage.getItem('list') | ||
? JSON.parse(localStorage.getItem('list')) | ||
: []; |
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.
근데 문기님이 제시해주신 코드에서, 첫번째 코드는 로컬스토리지에 아이템이 존재하는 경우(localStorage.getItem('list')가 true)에는 그 아이템들을 initialTodoData에 할당해줘야 하니까 [ ]가 아니라 JSON.parse(localStorage.getItem('list')가 되어야 할 것 같아요! 그래서 첫번째 코드랑 두번째 코드를 합쳐서 원래 예지님 코드처럼 localStorage.getItem('list') ? JSON.parse(localStorage.getItem('list')) : [ ]로 적는 게 더 간단하지 않을까 하는 저의 작은 의견,,, 그런데 문기님처럼 로지컬 연산자(&&, ||) 활용하는 거 너무 좋은 것 같아요!!
setList([...list, newTodo]); | ||
localStorage.setItem('list', JSON.stringify([...list, newTodo])); |
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.
근데 바로 윗줄에서 setList로 list state를 업데이트 해줬는데, localStorage.setItem('list', JSON.stringify([...list, newTodo]));가 아니라 localStorage.setItem('list', JSON.stringify(list));로 작성하면 혹시 안되나요?! 이건 저의 단순한 궁금증입니다....!
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.
어머 지금 그렇게 수정해서 돌려보니까 팽팽~ 잘 돌아가요!!! 예리한 지적... 감사합니닷!!! :)
|
||
const toggleTodo = (id) => { | ||
let toggledTodo = list.map((data) => | ||
data.id === id ? { ...data, completed: !data.completed } : data |
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.
불리언 이용해서 !로 true,false 토글하는 거 너무 좋은 것 같아요..!
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.
히히 감사합니당👍👍
let countTodo = 0; | ||
list.map((data) => (data.completed ? data : countTodo++)); |
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 React from 'react'; | ||
import styled from 'styled-components'; | ||
|
||
const Form = ({ getTodo, value, setValue }) => { |
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.
혹시 이 컴포넌트가 아니라 App.js에 const [value, setValue] = useState(''); 작성하신 이유가 있으신가요?? 이 컴포넌트에 바로 작성해주면 굳이 App컴포넌트에서 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.
헉 이건 미처 고려하지 못 했네요...😿 Form
컴포넌트에서 작성해 주는 것이 더 효율적일 것 같아요!!
@@ -0,0 +1,21 @@ | |||
import { createGlobalStyle } from 'styled-components'; |
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.
우와 저도 유용한 정보 알아가요!!❤
let editedTodo = list.map((data) => { | ||
if (data.id === Todo.id) { | ||
data.title = editedTitle; | ||
} | ||
return data; | ||
}); | ||
|
||
setList(editedTodo); |
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.
예지님이 App.js에 toggleTodo, deleteTodo 함수 만들어주셔서 list 상태를 업데이트해주신 것처럼, 이 부분도 굳이 이 컴포넌트로 list와 setList를 props로 받아오지 않고, App.js에 list 상태를 업데이트해주는 editTodo 함수를 만들어주고 이 컴포넌트에서 수정된 todo를 editTodo함수의 인자로 전달하는 방식을 사용하면 코드가 더 깔끔해질 것 같아요!
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.
허걱 저는 시야가 좁았나봐요...😿 그 방법이 더 효율적이고 가독성도 높을 것 같아요!!! 좋은 방법 알려주셔서 감사합니다!!
안녕하세요 이예지입니다.😆
리액트로 구현하니 지난 주 과제보다 확실히 편했지만(?) 예상치 못 한 에러가 자주 출동해서 고생 좀 했습니다. 하하
렌더링 최적화도 하고 싶었는데 무수히 쏟아지는 에러를 감당하지 못 하고... 네......😿
아쉬운 마음에 별 건 아니지만 투두 수정하는 기능을 추가했고 지난 주 과제에서 받은 피드백도 보완하려고 노력했습니당!!
리액트도 쉽지 않네요.. 쩝
Key Questions
1-1. Virtual-DOM은 무엇인가?
1-2. Virtual-DOM을 사용함으로서 얻는 이점은 무엇인가?
2. 미션을 진행하면서 느낀, React를 사용함으로서 얻을 수 있는 장점은 무엇인가?
3-1. React에서 상태란?
3-2. 상태를 어떻게 관리할 수 있는가?
4. Styled-Components 사용 후기
배포 링크
미션 결과