Conversation
src/App.ts
Outdated
|
|
||
| get filteredItems(): IItem[] { | ||
| const { isFilter, items } = this.state; | ||
| return items.filter(({ active }: { active: boolean }) => (isFilter === 1 && active) || (isFilter === 2 && !active) || (isFilter === 0) ) |
There was a problem hiding this comment.
items가 타입이 지정되어있으면 매개변수에 타입을 지정할 필요가 없어요. state에 타입을 지정해주는게 좋아보입니다!
제네릭도 사용할만하겠네요
There was a problem hiding this comment.
state에 Record<string, any>를 지정하면 초기값을 빈 객체로 지정할 때 에러가 떠 매개변수 타입을 IItem으로 지정했습니다
There was a problem hiding this comment.
제 주관적인 의견이에요. 피드백은 솔직하게 달도록 하겠습니다!
[구현]
구현은 황준일님 아티클과 비슷하게 했지만, 여기에 본인 생각이 조금 더 드러나도록 고민하면 좋을것 같아요. 예를 들어 네이밍에 대한 문제나, 특정 로직에 대해 바꿔보는거죠!
Important
초기 렌더시에만 mounted를 적용하려고 시도했던 것은 너무 좋습니다! 다만, 이런 것들을 검 해보면 더 좋을것 같아요. 현재 코드에서는 의도하신것처럼 동작하진 않네요ㅠㅠ
[타입스크립트]
전반적으로 타입스크립트가 약하다고 느껴졌어요. 타입에 대한 고민도 많이 해보면 좋을것 같아요. 지금은 타입을 위한 타입을 사용하는 느낌이 들었어요. 추가적으로 class와 typescript가 만나면 굉장한 oop 효과를 낼 수 있다고해요. 이러한 부분도 고민해보면 좋을것 같습니다
아래 코드를 사용할 때 dataset 속성을 태그마다 달아줘야 하는 게 비효율적이라고 생각해서 이벤트 위임 방식도 고려했지만, 이벤트 위임 방식보다 가독성이 좋을 것 같아 dataset 속성을 사용했습니다.
👉 HTML의 dataset은 HTML의 정보를 입력해 CSS, JS를 활용하는 목적에 있어요. 이걸 이벤트 위임 방식으로 한다는 것이 어떤건지 알려주실 수 있나요 ???
|
|
||
| deleteItem(id: number) { | ||
| const items = [...this.state.items]; | ||
| const index = items.findIndex(item => item.id === id) |
There was a problem hiding this comment.
filter를 사용하는게 더 직관적일 것 같아요
메서드의 이름을 보면서 네이밍 유추가 잘되었지만, 코드레벨에서도 고민해보면 좋을것 같습니다!
There was a problem hiding this comment.
한 번에 1개만 삭제할 수 있기 때문에 findIndex 메서드를 쓰는 게 효율적이라고 생각합니다.
There was a problem hiding this comment.
한 번에 1개만 삭제할 수 있기 때문에 findIndex 메서드를 쓰는 게 효율적이라고 생각합니다.
취향차이지만, 매개변수로 id를 받고, id는 고유하기 때문에 1개만 필터링 되지않을까? 생각이 드네요!
| } | ||
|
|
||
| toggleItem(id: number) { | ||
| const items = [...this.state.items]; |
There was a problem hiding this comment.
이 부분도 map을 사용하는게 조금 더 좋지 않을까? 라는 생각이 듭니다. map을 사용한다면 코드가 한줄로 표현도되지만, 선언적으로 작성하는 웹 개발 시대에는 지금처럼 부수효과를 일으키는 것보다 나은것 같아요!
물론! 깊은 복사를 하셔서 큰 상관은없지만 선언적이지 않다라고 느껴지네요
| // your Code | ||
| import App from './App' | ||
|
|
||
| class Main { |
There was a problem hiding this comment.
Main 클래스를 여러개의 인스턴스를 만들어도 이미 생성된 인스턴스를 가리키도록 싱글턴 패턴을 활용하면 좋지않을까 ? 라는 생각이드네요
| mounted() { | ||
| if (!this.isMounted) { | ||
| this.isMounted = true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
이 코드는 불필요해 보여요. 왜냐하면 App에서 mounted로 실행을 하게되면, ItemAppender, Filter, Items들의 인스턴스가 생성됨과 동시에 mounted가 실행됩니다. 그리고 투두를 추가하면 render가 동작하면서 mounted를 호출하게 되면서 자식 컴포넌트들도 호출됩니다.
즉 의도하신건 "불필요한 렌더링을 줄이자" 인것 같은데, 의도하것처럼 동작하지가 않아요. ItemAppender에 console.log()를 출력해보시면 새로운 투두가 생길 때마다 로그가 찍히는게 보일거에요. 따라서 큰 의미 없는 코드인것 같아요.
하지만 이런걸 개선하려고 했던 건 너무나도 좋은것 같습니다. 저는 그런 생각 못했어요!
|
@D5ng 피드백 확인했습니다. 상세한 리뷰 감사해요!! |
아하 이해했습니다! |
공부한 것
고민점