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

[3주차] 유세은 미션 제출합니다. #17

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

Conversation

SeieunYoo
Copy link
Member

@SeieunYoo SeieunYoo commented Apr 1, 2022

안녕하세요 프론트 15기 유세은입니다. 지난주차에서 미완성한 부분이 많아서 이번에 시간이 조금 걸렸습니다 ..typescript 로 변환하는 과정에서 많이 헤맸습니다(아직도 헷갈리는) 최대한 지난 번 리뷰를 반영하려고 노력했는데 잘됐는지 모르겠습니다 😂
**과제 마무리하고 나서 Context API를 시작해서 적용하지 못했습니다 ㅠㅠ 남은 시간동안 Context API 적용해보겠습니다

배포:
https://react-todo-15th-seieunyoo.vercel.app

보완한 부분(?)
-useEffect를 이용해서 실시간 시계 구현
-지난 번 리뷰 반영..?
-커밋 메시지 바르게 작성(?)
-styled-component 적용
-최대한 any 지양..

아쉬운점

-지난 번 코드에서 보완하려다보니 구성이 많이 아쉬웠습니다(다 갈아엎을뻔..) 최대한 중복된 부분은 줄여보려고 했는데 잘됐는지 모르겠습니다 😂 최적화를 위한 더 나은 방향 제시해주시면 감사하겠습니다 ^_^
-Context API 적용하지 못한..
-Item를 따로 빼려고 했는데 props 전달이 너무 복잡해져서 일단은 구현했습니다.

아직 부족한 점이 많아서 공부가 많이 필요한 것 같아요 제 코드 읽으실 분들께 미리 죄송하다는 말씀을 드립니다 ㅠ..ㅠ 감사합니다!

++)지난 주에 보낸 커밋은 JS에서 수정한 부분들이라서 타입스크립트는 최근에 추가한 14개 커밋으로 봐주시면 감사하겠습니다!

Copy link

@jdaeheon jdaeheon left a comment

Choose a reason for hiding this comment

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

안녕하세요, 이번 코드리뷰 파트너 15기 정대헌입니다.

세은님 코드 보면 다양한 시도를 하시는게 배울 점이 많은 것 같아요. styled component에서 props 전달받고 사용하시는거나, 공식 문서에서도 generic 쓰는 건 못본 것 같은데 멋있습니다~ 커스텀 훅을 자유롭게 사용하시는 점도 인상깊었습니다.
아직 TS에 대한 이해가 부족한 상태라 생산성 있는 코멘트를 많이 달지는 못했지만, 제가 고민했던 부분들 위주로 코멘트 남깁니다. 감사합니다~

Cat Wand

toDoList={toDoList}
setToDoList ={setToDoList}/>
</InputBox>
<hr/>
Copy link

Choose a reason for hiding this comment

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

처음보는 태그입니다, 새롭게 배워가요~

Copy link
Member Author

Choose a reason for hiding this comment

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

hr태그가 있으면 중간에 선이 생겨 구분이 가능해지더라구요!

<hr/>

<Todobox
type = "Todo"
Copy link

Choose a reason for hiding this comment

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

텍스트는 중괄호 없이도 prop 전달이 되는군요!

Comment on lines +4 to +20
interface ILIst {

color ?:string;
decoration ?:string;

}

export const ListTitle = styled.div
`font-size: 20px;
`
;

export const List = styled.li<ILIst>
`
color: ${(props) => props.color || "black"};
text-decoration: ${(props) => props.decoration || "none"};
`
Copy link

Choose a reason for hiding this comment

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

styled component에서 interface를 사용하고, 태그 요소에 제네릭 사용하시는게 신기하네요. 심지어 props까지 받아서 사용하시고 styled component 잘쓰시는 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

이상하게 인터페이스를 만들지 않으면 props가 전달이 안되더라구요 ,,! 그래서 구글링 했더니 styled-component에서 props를 받으려면 인터페이스를 만들어줘야 한다고 합니다(?) 사실 저도 잘 모르겠다는,,,,

Comment on lines 10 to 11
let firstColor = colors[Math.floor(Math.random() * colors.length)];
let secondColor = colors[Math.floor(Math.random() * colors.length)];
Copy link

Choose a reason for hiding this comment

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

동일한 colors 리스트라, const로 정의해도 잘 작동하는 것 같아요~

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 그렇네요~ 감사합니다!

Comment on lines +3 to +16

function Clock() {
const [time, setTime] = useState<string>();

function getTime (){
const date = new Date();

const hours = String(date.getHours()).padStart(2, "0");
const minutes = String(date.getMinutes()).padStart(2, "0");
const seconds = String(date.getSeconds()).padStart(2, "0");

let nowTime =`${hours}:${minutes}:${seconds}`;
setTime(nowTime);
}
Copy link

Choose a reason for hiding this comment

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

getTime을 functional component밖에서 정의하고 nowTime을 리턴하도록 해서
setTime(getTime()) 형태로 정의해도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

훨씬 코드가 깔끔해질 것 같습니다 감사합니다!!

Comment on lines +26 to +28
const Clock = styled.h1
`text-align: center`
;
Copy link

Choose a reason for hiding this comment

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

styled component도 functional component 밖에서 정의하는걸 추천하더라구요~

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 놓친 부분~ 감사합니다!!


function Done({type,setDoneToDoList ,ItemList ,setToDoList} : ItemList){

let list : string[] = ItemList;
Copy link

Choose a reason for hiding this comment

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

list를 따로 정의하는 이유가 있는 것 인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

ItemList를 props로 받아오고 바로 useToDo훅에 그 ItemList를 또 props로 전달하는데 이 과정에서 둘이 이름이 같아서인지(?) 제대로 전달이 안되더라구요 ㅠㅠ 그래서 한번 변수에 저장하고 했더니 전달이 잘 됐습니다!


function Inputform ({toDoList,setToDoList} : InputProps){

const{toDo, onChange ,inputToDo} = useInput();
Copy link

Choose a reason for hiding this comment

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

input까지 커스텀 훅을 사용하셨군요!

function ToDo({type,setDoneToDoList ,ItemList ,setToDoList}: ItemList){

let list : string[] = ItemList;
const {deleteButton, moveButton} = useToDo({type,setDoneToDoList ,list ,setToDoList});
Copy link

Choose a reason for hiding this comment

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

동사형으로 정의해 주시면 더 좋을 것 같아요~
moveItem()
deleteItem()

Copy link
Member Author

Choose a reason for hiding this comment

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

훨씬 가독성 있네요 감사합니다 !!

Comment on lines +4 to +35
const useToDo=({type, setDoneToDoList, list , setToDoList} : IProps) => {


const setToDo = (item : string, id :number) => {
if(type === "Todo")
setToDoList(list.filter((item, index) => id !== index)); //todolist remove
else
setDoneToDoList(list.filter((item, index) => id !== index)); //donelist remove

}

const setDoneToDo = (item : string, id :number) => {
if(type === "Todo")
setDoneToDoList((ItemList: Array<string>) => [item, ...ItemList]); //todolist 추가
else
setToDoList((ItemList: Array<string>) => [item, ...ItemList]); //donelist 추가
}
const deleteButton = (item : string, id :number) => {

setToDo(item,id);

};

const moveButton = (item : string, id :number) =>{

setDoneToDo(item,id); //토글이 일어나면 삭제하고 다른 리스트에 추가하기
setToDo(item,id);

};


return {deleteButton, moveButton};
Copy link

Choose a reason for hiding this comment

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

구조가 Reducer랑 느낌이 많이 비슷한 것 같아요, 사용해보시는 걸 추천합니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

대헌님 코드 보면서 리듀서 알아갑니다 ,,,굳

Copy link

@siwonblue siwonblue 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 +10 to +11
const [toDoList, setToDoList] = useState<string[]>([]);
const [doneToDoList, setDoneToDoList] = useState<string[]>([]);

Choose a reason for hiding this comment

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

useState 에도 타입설정을 해줘야하는지 지금 알았어요
세세한 디테일 좋네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

이상하게 타입을 지정해주지 않으면 제 코드에선 오류가 나더라구요😅아마 저는 props로 직접 전달해서 그런 것 같아요 ! 보통 생략하는 경우도 많더라구요!

Comment on lines +4 to +9
interface ILIst {

color ?:string;
decoration ?:string;

}

Choose a reason for hiding this comment

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

저는 이 부분 신경 못 썼는데 styled-components 에도 꼼꼼하게 해주셨네요

let firstColor = colors[Math.floor(Math.random() * colors.length)];
let secondColor = colors[Math.floor(Math.random() * colors.length)];

document.body.style.background = `linear-gradient(${firstColor}, ${secondColor})`;

Choose a reason for hiding this comment

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

새로고침 할 때 마다 배경이 바뀌는 부분 예쁘네요

Copy link
Member Author

Choose a reason for hiding this comment

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

다른 컬러들도 추가해보겠습니다 😄

Copy link

@S-J-Kim S-J-Kim left a comment

Choose a reason for hiding this comment

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

세은님 안녕하세요

프론트엔드 멘토 김선종입니다.

타입스크립트 어떠셨나요? 저도 타입스크립트를 맨 처음해봤을 때 타입을 대체 어떻게 맨날 찾아다녀야 하지 하고 막막했던 경험이 있습니다. 근데 결국 개발을 하다보면 익숙해지더라구요,, 코드 로직 자체는 참 깔끔했는데, 이번에 ESLint와 Prettier를 설정하시면서 prettier 설정이 깨진것 같아 보입니다. 보통 이런 config파일간의 의존관계를 해결하는 것 또한 개발을 진행하면서 중요한 일 중에 하나인데요, 이번을 계기로 세은님께서 이 문제를 해결해보시고, 각각의 config 파일들의 역할에 대해 더욱 더 잘 이해할 수 있는 계기가 되었으면 좋겠습니다.

과제 하느라 고생하셨습니다

"plugin:@typescript-eslint/recommended",
"prettier/react",
"prettier",
"airbnb",
Copy link

Choose a reason for hiding this comment

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

에어비앤비 룰 좋아요

Comment on lines +23 to +35
<Ul>
<ListTitle>✍️To Do({list.length})</ListTitle>
{
list.map((item, id) => (
<List key={id}>
<div>
<Button onClick={() => moveButton(item,id)}>✔️</Button>
{item}
<Button onClick={() => deleteButton(item,id)}>❌</Button>
</div>
</List>
))}
</Ul>
Copy link

Choose a reason for hiding this comment

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

헉,,, eslint를 설정하시면서 prettier랑 충돌이 있던것 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅠㅠ다시 한번 수정해보겠습니다 감사합니다 ㅠㅠ 다른 분들이 코드 보시기 힘드셨겐군뇨,,죄송합니다😅

export interface IProps{

type : string;
setDoneToDoList: Dispatch<SetStateAction<string[]>>;
Copy link

Choose a reason for hiding this comment

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

이거 타입 찾느라 고생좀 하셨겠군요..

Copy link
Member Author

Choose a reason for hiding this comment

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

😅구글링 하다보니까 저랑 비슷한 분들이 많으시더라구요 ,,

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.

5 participants