-
Notifications
You must be signed in to change notification settings - Fork 10
[2주차] 장자윤 과제 제출합니다. #3
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
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주차 과제도 고생 많으셨습니다!!
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 파일 하나에서 모든 theme 제외 모든 컴포넌트에 대한 styled component가 정의되고 있는 것으로 보이는데, 가독성과 유지보수를 위해 파일을 분리해 보시는 것이 좋을 것 같습니다!
const dd = String(selectedDate.getDate()).padStart(2, '0'); | ||
|
||
// selectedDate = Date 객체 -> 문자열로 변경 | ||
const stringDate = `${yyyy}-${mm}-${dd}`; |
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.
지금 가장 상단의 오늘 날짜를 받는 부분에서도 같은 형식으로 날짜를 반환하는 로직이 있는 것 같습니다! 이렇게 여러 번 사용되는 로직은 util 함수로 따로 빼서 관리해 보는 것이 어떨까요?
margin-bottom: 0.3rem; | ||
`; | ||
|
||
const PrevWeekButton = styled.button` |
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.
PrevWeekButton, PrevButton, NextButton, NextWeekButton 네 가지는 대부분의 속성이 같고 세부 사항만 조금 다른 것 같습니다
자윤 님께서 저번 주차에 발표해 주셨던 것 같은데, 공통 스타일을 NavButton 같이 묶고, styled(NavButton) 과 같은 형태로 사용하면 중복을 줄일 수 있을 것 같아요!
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.
공통 스타일을 NavButton 컴포넌트로 만들고 차이점만 props로 조정하면 더 효율적일 것 같습니다.
border: 1px solid grey; | ||
border-radius: 5px; | ||
|
||
&: placeholder { |
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.
&: placeholder { | |
&::placeholder { |
<AppendButton onClick={addTodo}>+</AppendButton> | ||
</InputContainer> | ||
{list.map((todo, index) => ( | ||
<ListContainer key={index}> |
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에서 key 값에 index를 사용하지 않는 것을 권장하고 있습니다! id 같은 고유한 값으로 설정하신다면 더 좋을 것 같습니당
(리액트 공식 문서 참고)
https://react.dev/learn/rendering-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.
저도 왜 안되는지 잘 이해하지 못했는데, 위 글을 읽고 이번 과제에 고쳤었습니다! 한번 꼭 읽어보세요!
// Todo 추가 | ||
const addTodo = () => { | ||
const todoInputText = todoInput.trim(); | ||
if (!todoInputText) 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.
UX 향상을 위해 alert나 error UI 같이 왜 추가가 안 되는지 사용자가 알 수 있는 장치가 있으면 좋을 것 같아요!
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.
}; | ||
// 한글 입력 enter 처리 | ||
const handleKeyDown = (e) => { | ||
if (e.key === 'Enter' && !e.nativeEvent.isComposing) addTodo(); |
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주차 과제 리뷰를 맡게 된 손주완입니다!
react 폴더 구조
이 글과 21기 react-todo-21nd pr을 보시면 예쁘게 폴더구조가 형성된 react 프로젝트를 볼 수 있으니 다음번에 한 번 적용해보셨으면 좋겠습니다!
제 코드리뷰 읽어보시고 질문이 있으시면 댓글 달아주세요! 이번주도 고생많으셨습니다!
const toggleTheme = () => | ||
setIsDark((prev) => { | ||
const themeMode = !prev; | ||
sessionStorage.setItem('themeMode', themeMode ? 'dark' : '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.
현재 테마 설정이 sessionStorage에 저장되고 있는 것 같습니다.
UX 개선을 위해 localStorage로 변경하는 걸 추천드립니다! 사용자가 브라우저를 재방문해도 다크/라이트 테마가 유지될 수 있으니까요!
}); | ||
}; | ||
|
||
// 오늘 날짜로 (로고클릭) |
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.
로고를 클릭하면 오늘 날짜로 되돌아 올 수 있다는 사실을 처음 방문한 사용자는 알아차리기 힘들거 같습니다!
그리고 통상적으로 로고를 클릭하면 메인 페이지(초기 페이지)로 간다고 생각하기에, 페이지를 분할했을 때 더 아쉬울 거 같기도 하구요!
<AppendButton onClick={addTodo}>+</AppendButton> | ||
</InputContainer> | ||
{list.map((todo, index) => ( | ||
<ListContainer key={index}> |
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.
저도 왜 안되는지 잘 이해하지 못했는데, 위 글을 읽고 이번 과제에 고쳤었습니다! 한번 꼭 읽어보세요!
background-color: ${(props) => props.theme.background}; | ||
color: ${(props) => props.theme.color}; | ||
font-family: Pretendard, sans-serif; | ||
} |
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.
transition: background-color 0.3s ease, color 0.3s ease;
여기에 트랜지션 코드 한 줄만 넣으시면 다크/라이트 모드 전환이 더 예뻐질 거 같습니다!!
const savedTodos = localStorage.getItem('todos'); | ||
return savedTodos ? JSON.parse(savedTodos) : {}; | ||
}); | ||
const list = todos[currentDate] || []; |
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.
변수명을 list 대신 currentTodos로 하시면 더 좋을거 같습니다!
// Todo 추가 | ||
const addTodo = () => { | ||
const todoInputText = todoInput.trim(); | ||
if (!todoInputText) 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.
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와 prettier을 같이 사용할 때 참고해 볼 만한 글 남겨드립니다. (충돌방지 셋팅도 읽어 보시면 좋을거 같아요)
eslint와prettier설정방법
결과 화면
느낀점 및 배운점
Review Questions
Virtual DOM (가상 DOM)
가상 DOM 원리
가상 DOM 이점
리액트 렌더링 최적화
React.memo()
-> 컴포넌트 레벨에서, props가 같으면 리렌더링 건너뜀
useMemo()
-> 값(계산 결과) 캐싱
useCallback()
-> 함수 자체 캐싱
React 컴포넌트 생명주기
클래스 컴포넌트
함수형 컴포넌트 + Hook
useEffect Hook이 클래스 생명주기 메서드 대체
useEffect(()=>{...}, [])
: componentDidMount (처음 렌더링 시 1회 실행)useEffect(()=>{...}, [deps])
: componentDidUpdate (deps 변경 시 실행)useEffect(()=>{return()=>{...}}, [])
: componentWillUnmount (cleanup)배포 링크 : react-todo-22nd.vercel.app
+ 커밋 메시지 'fix: merge 충돌 해결' 부분에서 VanillaJS->React로 변경하였는데 merge 충돌 문제도 함께 해결하면서 커밋메시지를 저렇게 남겼네요,, 참고해주시면 감사하겠습니다!!