Skip to content
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주차] 김현민 미션 제출합니다. #6

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

wokbjso
Copy link

@wokbjso wokbjso commented Sep 22, 2023

배포링크

이전 코드들을 그대로 리액트로 변경하였습니다. favicon 도 추가하였고, useMemo 를 통해 input 입력때마다 todo들이 전부 다 re-rendering 되는 과정을 방지하였습니다. 또한 다른 라우트로 이동 시 자동으로 홈으로 navigate 되게 처리 해주었습니다.

Key Questions

  1. Virtual-DOM은 무엇이고, 이를 사용함으로서 얻는 이점은 무엇인가요?
  • Virtual-DOM 이란 가상 DOM 으로 실제 DOM 을 추상화 한 개념이다.

    이전 과제에서 미리 언급을 했었는데 HTML 태그들이 트리 형태의 DOM 구조로 저장이 되는데 개발자가 코드를 수정할 때 마다
    DOM 에 직접 접근하는 것은 매우 비효율적인 작업이다.

    그래서 수정하기 전의 Virtual DOM과 수정 후의 Virtual DOM을 비교하여 변경된 요소들만 파악한 후, 그 요소들만 직접 DOM
    에 접근하여 수정해주는 작업을 거치게 된다. 이로써 웹사이트의 효율적인 수정을 이끌어 낼 수 있다.

  1. 미션을 진행하면서 느낀, React를 사용함으로서 얻을수 있는 장점은 무엇이었나요?
  • 생 HTML, CSS, 자바스크립트로 작업을 하다가 React를 사용하니까 너무 많은 장점들을 느낄 수 있었다.

    우선 내가 가장 편리하다고 느낀 점은 컴포넌트 단위로 작업을 할 수 있다는 점이었다. 컴포넌트 단위로 작업을 하니 props 만
    넘겨주면 재활용성이 편리해서 웹사이트를 구성하는데 굉장히 편리했다.

  1. React에서 상태란 무엇이고 어떻게 관리할 수 있을까요?
  • 상태란 한마디로 데이터의 현재 값을 말한다. 리액트에서는 useState 로 상태를 관리하게 되는데, 변수와 그 변수를 변경시켜줄
    수 있는 setter 함수로 이루어진다.

    setter 함수로 해당 변수를 변경해주면 리액트에서는 이를 감지하여 해당 변수가 쓰인 부분만 다시 re-rendering 해주는 과정을
    거치게 된다.

    이로써 변경된 페이지 전체를 다시 불러오는 SPA 형식에서 수정된 부분만 다시 불러오는 훨씬 효율적인 방법으로 작업할 수 있
    다.

  1. Styled-Components 사용 후기 (CSS와 비교).
  • 평소에도 내가 즐겨쓰는 CSS-IN-JS 라이브러리인데 이번에 이전 과제와 비교하면서 사용해보니 얼마나 좋은 라이브러리인지
    다시 한번 깨닫게 되었다.

    우선 class 및 id 를 내가 직접 세팅해주지 않아도 자동으로 설정되고, 내가 원하는 이름으로 태그를 설정할 수가 있어 코드 가독
    성도 훨씬 좋아졌다.

    또한 css 파일을 따로 만들어주지 않아도 돼서 파일을 오가면서 css 작업을 해주지 않아도 돼서 굉장히 코딩 작업이 편리했다.

Copy link
Collaborator

@westofsky westofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 프론트엔드 운영진 배성준입니다!!
Todo로직을 깔끔하게 작성하시고 rem단위를 너무 잘쓰셔서 재미있게 리뷰했습니다~~
이번주 과제 고생 많으셨어요 다음 과제도 파이팅입니다

Comment on lines +17 to +18
const updatedTodoLists = [...todoLists];
updatedTodoLists.splice(index, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분은 filter함수를 이용해서

Suggested change
const updatedTodoLists = [...todoLists];
updatedTodoLists.splice(index, 1);
const updatedTodoLists = todoLists.filter((_, idx) => idx !== index);

처럼 사용할 수 있을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 의견 감사드립니다~!!

return (
<Routes>
<Route path="/" element={<Todo />} />
<Route path="/*" element={<Navigate to="/" />} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 디테일 너무 좋아요

}
`;

const TodoListWrapper = styled.div``;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

영어를 길게 입력한 경우 자동 줄바꿈이 되지 않아

Suggested change
const TodoListWrapper = styled.div``;
const TodoListWrapper = styled.div`
word-break : break-all;
`;

속성을 추가하는 건 어떨까요 ??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 생각하지 못했네요 ㅎㅎ 감사합니다!!

Comment on lines +1 to +11
export const getTotalTodos = (todoLists) => {
return todoLists.length;
};

export const getCheckedTodos = (todoLists) => {
return todoLists.filter((todoList) => todoList.checked).length;
};

export const getProceedingTodos = (todoLists) => {
return getTotalTodos(todoLists) - getCheckedTodos(todoLists);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사람마다 다르지만 하나의 return 만 있을때에는 중괄호와 return 키워드를 생략할 수도 있습니다!

Suggested change
export const getTotalTodos = (todoLists) => {
return todoLists.length;
};
export const getCheckedTodos = (todoLists) => {
return todoLists.filter((todoList) => todoList.checked).length;
};
export const getProceedingTodos = (todoLists) => {
return getTotalTodos(todoLists) - getCheckedTodos(todoLists);
};
export const getTotalTodos = (todoLists) => todoLists.length;
export const getCheckedTodos = (todoLists) => todoLists.filter((todoList) => todoList.checked).length;
export const getProceedingTodos = (todoLists) => getTotalTodos(todoLists) - getCheckedTodos(todoLists);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 감사합니다😄

Comment on lines +6 to +15
const [checkState, setCheckState] = useState(todo.checked);
const checkboxChanged = (e) => {
const updatedTodoLists = [...todoLists];
updatedTodoLists[index] = {
...updatedTodoLists[index],
checked: !updatedTodoLists[index].checked,
};
setTodoLists(updatedTodoLists);
setCheckState((prev) => !prev);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [checkState, setCheckState] = useState(todo.checked);
const checkboxChanged = (e) => {
const updatedTodoLists = [...todoLists];
updatedTodoLists[index] = {
...updatedTodoLists[index],
checked: !updatedTodoLists[index].checked,
};
setTodoLists(updatedTodoLists);
setCheckState((prev) => !prev);
};
const checkboxChanged = () => {
const updatedTodoLists = [...todoLists];
updatedTodoLists[index].checked = !updatedTodoLists[index].checked;
setTodoLists(updatedTodoLists);
};

어차피 setTodoLists로 다시 받아온다면 checked도 바뀐 채로 넘어와서 checkState는 따로 관리하지 않아도 될 것 같습니다! 또한 todoLists가 바뀌면서 rendering이 되고 checkState가 바뀌면서 다시 rendering이 될 것 같아 불필요하게 두번 렌더링이 될 것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

최적화에 더 신경쓰도록 하겠습니다!! 감사합니다 ㅎㅎ

Comment on lines +14 to +20
width: 22rem;
height: 9rem;
padding: 2rem;
display: flex;
justify-content: center;
align-items: center;
border-radius: 1rem;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rem단위를 자유자재로 사용하시다니 너무 부러워요

Comment on lines +10 to +78
const [todoText, setTodoText] = useState("");
const [todoLists, setTodoLists] = useState(getTodo());
const todoInputChanged = (e) => {
setTodoText(e.target.value);
};
const enterPressed = (e) => {
if (e.key === "Enter") {
addTodoClicked();
}
};
const addTodoClicked = () => {
if (todoText.trim() === "") {
alert("todo를 입력하세요");
setTodoText("");
return;
}
setTodoLists((prev) => [
...prev,
{ todoText: todoText.trim(), checked: false },
]);
setTodoText("");
};
useEffect(() => {
setTodo(todoLists);
}, [todoLists]);
return (
<TodoWrapper>
<TodoHeader>
<HeaderTitle>
<span>TO-DO LIST</span>
</HeaderTitle>
<HeaderBtnWrapper>
{headerBtnState.map((btnState) => (
<TodoCountBtn
key={btnState.text}
btnState={btnState}
addClass="margin:0 1.5rem;"
todoLists={todoLists}
/>
))}
</HeaderBtnWrapper>
</TodoHeader>
<TodoMainBoard>
<AddTodoWrapper>
<AddTodoInput
placeholder="입력하세요"
value={todoText}
onChange={todoInputChanged}
onKeyDown={enterPressed}
/>
<AddTodoBtn onClick={addTodoClicked}>
<span>+</span>
</AddTodoBtn>
</AddTodoWrapper>
<TodoListWrapper>
{todoLists.map((todo, index) => (
<EachTodo
key={todo.todoText + index}
index={index}
todo={todo}
todoLists={todoLists}
setTodoLists={setTodoLists}
/>
))}
</TodoListWrapper>
</TodoMainBoard>
</TodoWrapper>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header, Input, ListView가 한 페이지에 있어서 리스트 관련한 동작을 할 때 Header와, Input 모두 렌더링이 되는 것 같아요 좀 더 컴포넌트 단위를 쪼개면 좋을 것 같습니다! 또한 이렇게 되면 Input을 입력할때마다 전체가 렌더링이 돼요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작업 후 추가적으로 분리 할 생각이였는데 시간상 그냥 제출하게 되었네요 ㅠ 담에는 꼭 분리하도록 하겠습니다!!

Copy link

@oooppq oooppq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔한 코드 잘 봤습니다~~ 각 디렉토리별로 역할이 명확하게 구분되어 있고, 네이밍도 깔끔해서 리뷰하기 편했던 것 같아요! styled-components에 상당히 능숙하신 것 같은데 현민님 코드 보면서 styling 하는 부분 많이 배워야 할 것 같아요 수고하셨습니다~~

return (
<Routes>
<Route path="/" element={<Todo />} />
<Route path="/*" element={<Navigate to="/" />} />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메인 페이지 이외의 접근을 막는 디테일 좋은 것 같아요~~


ReactDOM.render(
<React.StrictMode>
<App />
<ThemeProvider theme={theme}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThemeProvider로 스타일을 적용한 부분 저도 배워야 겠습니다..👍

<AddTodoInput
placeholder="입력하세요"
value={todoText}
onChange={todoInputChanged}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onChange 이벤트마다 state를 변경하게 하면 컴포넌트가 계속 리렌더링이 되는데, todo Input state와 관련이 없는 부분은 새로운 컴포넌트로 분리하는게 효율적일 것 같아요! 저는 react의 memo와 useCallback을 사용해서 렌더링 최적화를 구현했는데 한 번 시도해보면 좋을 것 같습니다~~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시간이 부족하여 분리를 못했는데 대균님처럼 memo와 useCallback 사용해서 꼭 분리해보도록 하겠습니다 감사합니다 ㅎㅎ

@@ -0,0 +1,68 @@
import { createGlobalStyle } from "styled-components";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createGlobalStyle 잘 배워갑니다!!

Comment on lines +22 to +23
setTodo(todoLists);
}, [todoLists]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 localStorage에 데이터 업데이트 해주는 logic을 add, delete 함수 안에 포함했는데, 이렇게 useEffect를 사용하면 훨씬 직관적이고 편리할 것 같네요!

Comment on lines +6 to +23
const [checkState, setCheckState] = useState(todo.checked);
const checkboxChanged = (e) => {
const updatedTodoLists = [...todoLists];
updatedTodoLists[index] = {
...updatedTodoLists[index],
checked: !updatedTodoLists[index].checked,
};
setTodoLists(updatedTodoLists);
setCheckState((prev) => !prev);
};
const deleteTodo = () => {
const updatedTodoLists = [...todoLists];
updatedTodoLists.splice(index, 1);
setTodoLists(updatedTodoLists);
};
useEffect(() => {
setTodo(todoLists);
}, [todoLists]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 logic이 별루 없는데, logic 복잡해지면 디버깅을 위해 view와 분리하여 다른 파일에서 작성해봐도 가독성에 좋을 것 같아요~~

Copy link
Author

@wokbjso wokbjso Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 view 와 controller 는 분리하는게 나을 것 같네요!! 피드백 감사합니다~!!!

Copy link

@rmdnps10 rmdnps10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 분들이 워낙 피드백을 잘해주셔서, 코드 로직 부문에서 더 개선할 점에 대해서는 따로 짚지 못했습니다ㅎㅎ..
현민님 코드리뷰 하면서 저도 많이 배웠습니다, 이번 주 과제 하느라 고생 많으셨습니다!

Comment on lines +5 to +8
return (
<Routes>
<Route path="/" element={<Todo />} />
<Route path="/*" element={<Navigate to="/" />} />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 메인페이지 이외 접근을 막을 수도 있었군요 ㅎㅎ 디테일 감탄했습니다

Comment on lines +1 to +14
const theme = {
colors: {
allBtn: "wheat",
doingBtn: "skyblue",
doneBtn: "pink",
deleteBtn: "gray",
},
styles: {
background: "linear-gradient(to right, #dddddd, #ffffff, #dddddd)",
border: "linear-gradient(to left, white, black)",
},
};

export default theme;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스타일링할 요소들을 theme.js 안에서 직관적인 변수를 통해 선언하고 , ThemeProvider 을 통해 전역으로 공급해서 스타일링하는 방식이 되게 깔끔한 것 같아요!

Comment on lines +138 to +140
width: 8rem;
height: 7rem;
padding: 1rem;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반응형을 고려하여 rem 단위로 작업하신 점이 되게 인상적입니다 😊

return;
}
setTodoLists((prev) => [
...prev,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 기존의 배열을 props 으로 받아 spread 연산자를 통해 상태를 업데이트 했었는데, 상태 업데이트 함수를 쓸 때, (prev) 를 매개변수를 받아 이전의 상태를 불러올 수 있었군요... 현민님께서 하신 방법이 더 실용적일 것 같습니다!

span {
font-size: 6rem;
font-family: "GangwonState";
text-decoration: ${(props) => (props.$checked ? "line-through" : null)};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TodoText 컴포넌트에 대해서 스타일링을 props에 따라 다르게 하셨네요! 스타일 컴포넌트 활용을 잘 하시는 것 같습니다 ㅎㅎ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants