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주차] 정대헌 미션 제출합니다. #21

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

Conversation

jdaeheon
Copy link

@jdaeheon jdaeheon commented Apr 1, 2022

링크: https://react-todo-15th-asiloveyu.vercel.app/

comment
지난번과 웹페이지는 동일합니다! 다만 reducer에서 context API로 이사 한번하고 미처 옮기지 못한 css도 모두 옮겼습니다. context API와 typescript를 접목시키는데 시간이 많이 걸린 것 같아요. 그나마 첨부주신 링크를 보며 조금이나마 수월하게 한 것 같습니다. TS...자료형만 정의하는 줄 알았더니 interface, 상속, 타입캐스팅 등등 정적 타입 언어의 망령이 따라오는군요. 그래도 확실히 대규모 프로젝트에서 왜 사용해야 하는지 조금이나마 이해하게 된 것 같습니다. TS의 개념 중 아직 10% 정도만 활용한 것 같지만, 맛보는데 좋은 기회였던 것 같아요. 감사합니다.

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.

안녕하세요 대헌님 코드 리뷰 파트너 전시원입니다.
역시 디테일한 CSS 처리와 깔끔한 코드가 인상적이었습니다.
특히 리덕스 사용해서 데이터 관리하신 점 좋았습니다. 아는 게 없지만 최대한 도움 드리고자
몇 자 남겨봤습니다. 그럼 스터디때 뵙겠습니다

Comment on lines +13 to +19
<div className="background">
<div className="container">
<InputContainer />
<ListItemContainer title={"해야할 일"} listType={ItemType.Todo} />
<ListItemContainer title={"완료한 일"} listType={ItemType.Done} />
</div>
</div>

Choose a reason for hiding this comment

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

구조가 깔끔해서 보기 좋네요

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 +28 to +30
&:hover {
box-shadow: 0 0 10px rgba(255, 255, 255, 0.6);
}

Choose a reason for hiding this comment

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

역시 디테일 최고네요

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 +66 to +72
const addItem = (uid:string, itemType: ItemType, itemContent: string) => {
dispatch({
type: ActionType.ADD,
payload: { id: uid, type: itemType, content: itemContent },
});
};

Choose a reason for hiding this comment

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

리덕스 적용하신 것까지 멋있어요

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 7
const initialItems: State = JSON.parse(localStorage.getItem("list") || "") || [];
// JSON.parse는 string을 인자로 받으나 localStorage의 리턴 타입은 string or null 이므로 ""을 추가한다

Choose a reason for hiding this comment

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

저도 이렇게 했는데 초기에 로컬스토리지에 값이 없으면 에러가 나요 ㅠㅠ
지금 배포해주신 링크도 로컬스토리지 지우고 새로고침하면 에러나서 아예 아무것도 뜨지 않네요
아래 함수가 좀 이상하긴 한데, 저는 대충 저렇게 오류 해결했습니당

const getLocalValue = (listName = 'list') => {

  const value = localStorage.getItem(listName);
  let localData;
  if (typeof value === 'string') {

      localData = JSON.parse(value);
  }else if(value === null){

      localData = []
  }

  return localData
};

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 시원님, 말씀 주신대로 localStorage 삭제하니 에러가 나네요!
시원님 코드 참고해서 이렇게 수정했습니다. 공유해주셔서 감사해요~

Suggested change
const initialItems: State = JSON.parse(localStorage.getItem("list") || "") || [];
// JSON.parse는 string을 인자로 받으나 localStorage의 리턴 타입은 string or null 이므로 ""을 추가한다
const initialItems: State = JSON.parse(localStorage.getItem("list") || "[]");
// JSON.parse는 string을 인자로 받으나 localStorage의 리턴 타입은 string or null 이므로 ""을 추가한다

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.

대헌님 안녕하세요

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

타입스크립트와 동시에 Flux 패턴으로 리팩토링까지 하시고,, 이번주에 많은 공부가 되셨을 것 같습니다. 하지만 dispatch에서 사용가능한 액션들을 여러개의 컴포넌트에서 따로따로 불러와 사용하셨는데요, 저는 개인적으로 이런 것들을 한 모듈에서 모아 관리하면 유지보수성에 더 좋지 않을까 생각해봅니다. (저의 경우 이럴 때 커스텀 훅을 사용했습니다) 그래도 코드가 깔끔해서 리뷰를 아주 편하게 할 수 있었습니다. 대헌님의 다음 과제가 기대되는 부분이네요.

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

src/Objects.ts Outdated

export enum ItemType {Todo, Done}

export type Action = {type: ActionType, payload: any}
Copy link

Choose a reason for hiding this comment

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

액션은 정확하게 작성해주시는게 좋습니다. 각 action별로 payload의 타입이 전부 다를텐데, 이걸 컴파일 타임에 검사를 안해주면 런타임에 잘못된 코드가 들어왔을 때 대비가 안될 수 있죠.

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다, 제가 any로 두고 고치질 않았군요. 수정하겠습니다 :)

Suggested change
export type Action = {type: ActionType, payload: any}
export type Action = {type: ActionType, payload: Item}

>
<ItemTitle>
{/* listType의 done/todo에 따라 del 태그 삽입 */}
{item.type === ItemType.Done ? <del>{item.content}</del> : item.content}
Copy link

Choose a reason for hiding this comment

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

태그로 감싸는것도 새로운 방법이네요. 하지만 실제로는 css로 처리하는게 렌더링 성능에서 더 유리하다고 합니다!

Copy link
Author

Choose a reason for hiding this comment

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

조언 감사합니다~ 저는 오히려 css로 처리하는 방법을 생각하지 못했네요, 참고하겠습니다.

<FormInput
type="text"
id="todoItem"
placeholder="오늘은 행복하길 바래"
Copy link

Choose a reason for hiding this comment

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

깨알 감동

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 +9 to +16
case ActionType.MODIFY:
return [
...state.map((item: Item) =>
item.id === action.payload.id
? { ...item, type: item.type === ItemType.Todo ? ItemType.Done : ItemType.Todo }
: item
),
];
Copy link

Choose a reason for hiding this comment

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

리듀서는 항상 순수함수로 작성하시는게 좋아요. 같은 iditem이 들어와도 type 여부에 따라 리듀서가 리턴하는 값이 달라지죠? 아직은 애플리케이션의 상태가 작지만, 어플리케이션의 규모가 커지면 이런 부분에서 디버깅이 고통스러워질 수 있습니다

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

@YoommY2 YoommY2 left a comment

Choose a reason for hiding this comment

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

안녕하세요 대헌님 이번 코드 리뷰 파트너가 된 유세은입니다

리액트에 대햇 능숙하신게 보이는 과제였습니다,,! 코드가 깔끔해서 리뷰어 입장에서 보기 좋았습니다 제 코드는 그렇지 않아서 대헌님을 보면서 많이 배워가는 것 같습니당 Context API 를 적용하신 점이 인상 깊습니다 ,, 전 아직 공부를 더 해야할 것 같습니다 사실 타입스크립트를 잘 모르는 편이라 조금이나마 코멘트 남겨봤습니다 과제 하느라 고생 많으셨습니다 !! 이따 스터디 시 때 뵙겠습니다 ^_^

Comment on lines 1 to 13
const AppReducer = (state, action) => {
//DELETE = 아이템 삭제, ADD = 아이템 추가, MODIFY = 아이템 type 수정(done/todo)
switch (action.type) {
case "DELETE":
return [...state.filter((item) => item.id !== action.payload.id)];
case "ADD":
return [...state.concat([action.payload])];
case "MODIFY":
return [
...state.map((item) =>
item.id === action.payload.id
? { ...item, type: item.type === "todo" ? "done" : "todo" }
: item
Copy link

Choose a reason for hiding this comment

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

리듀서를 사용하니까 로직이 깔끔해지는 것 같습니다 ! 저도 적용해봐야겠군뇨

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 21 to 22
payload: { id: uid, type: itemType, content: itemContent },
});
Copy link

Choose a reason for hiding this comment

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

uid는 유니크한 id를 생성하는 건가요?! 좋은 방법인 것 같습니다 배워갑니당,,

Copy link
Author

Choose a reason for hiding this comment

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

�아 그냥 변수 이름만 uid이고 사실 유니크 하진 않습니다, uuid를 사용해야하는 걸로 알고 있어요!

Comment on lines +74 to +75
const ListItem = ({ item }: {item: Item}) => {
const dispatch = useContext(DispatchContext);
Copy link

Choose a reason for hiding this comment

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

const ListItem = ( {item} : Item)
이렇게는 작성하면 혹시 오류가 날까요?!,,

Copy link
Author

Choose a reason for hiding this comment

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

말씀하신 대로 수정해보았는데, prop를 2개를 받고 있어서 오류가 나는 것 같아요!
map 사용 시 필요한 key를 prop으로 받고 있습니다~

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