[react-todo-list step1] 강동현 미션 제출합니다. #10
Conversation
There was a problem hiding this comment.
@mintcoke123 👋🏻
반갑습니다, 오 프로젝트가 TypeScript로 가꾸어져 있군요 👍🏻 GREEDY 코드 리뷰에서 TypeScript를 쓰시는 분을 리뷰하는 건 처음이군요.
1️⃣ 답변
요구사항을 꼼꼼히 안 읽고 시작해 신나게 zustand 써서 프로젝트를 다 만들고 다시 리팩터링하는등 불필요한 과정이 있었지만요..🥲
어 Zustand같은 상태 관리 라이브러리 쓰면 안 된다고 요구사항에 적혀 있어요? 이 저장소에서는 그런 이야기가 없어서 몰랐네요
그래서, 이번 미션에서 다음과 같은 부분들을 신경써서 적용하였습니다.
- 컴포넌트, 기능의 최소단위로의 분리
- 타입분리, 상수화
수고하셨어요, 제가 보았을 때는 컴포넌트의 구성은 좋은 것 같고 역할에 맞게 적절히 쪼개져 있는 것 같아요. 다만 미션에서 강제하는 컴포넌트도 많았는지라 구조를 완전히 원하는 대로 커스터마이징하기는 좀 어려우셨을 것 같기도 하더라고요.
타입의 경우 역할에 맞게 잘 관리해 주신 것 같아요! interface를 사용해서도 여러 함수들을 적절히 extends를 이용해 사용해 주신 것도 잘 보았습니다.
다만 상수화의 경우에는 상수 오브젝트들의 타입을 모두 TypeScript가 추론해 주고 있어서 따로 인터페이스는 없어도 되지 않았을까 하는 생각이 들어요. 그리고 타입 자체만 보았을 때는 상수치고 "변경해서는 안 된다는" 의미가 아직 부족한 듯 보였어요. 그래서 이 부분은 코멘트 드려봤습니다. 타입도 종류가 많은지라 "유틸리티 타입" 등을 키워드로 구글링 해 보시면 다양한 종류의 타입들이 보일 거에요.
ex) loadTodos(), saveTodos(): UI와 무관한 함수는 컴포넌트와 깊게 연결되어 있지 않으면 분리.
해당 도메인 인상깊게 잘 봤습니다. 로컬 스토리지를 이용하셨던데 그 시도 자체도 좋고요, 로컬 스토리지의 값 또한 오염될 수 있음을 생각하여 검증 로직 넣어주신 것도 좋았습니다. 다만 여전히 검증에서 놓치는 값들도 많이 보인 점은 개선될 여지가 보였어요. 이 부분도 코멘트 달아두었습니다!
또한, 스타일의 일관성을 위해 색상값 역시 상수화로 관리했습니다.
오 잘 하셨습니다, styled-components에서 색상값을 하드코딩하여 관리하는 경우 휴먼 에러가 많이 날 수 있고, 같은 색상을 여러 곳에 쓰는 경우 이후 수정이 점점 어려워질 거라 생각해요. HEX 코드를 봤을 때 이게 당장 무슨 색상인지도 알기 어렵고요. 그래서 이 부분에 있어서 색상을 상수화하여 도입하신 건 정말 좋은 선택이라 생각하며, 이 선택으로 얻을 수 있는 장점들이 많이 보입니다.
만약 이 색상들을 전역 수준으로 자주 이용해야 한다면, 이 방법 도 활용해 보실 수 있을 겁니다.
저번 '점심 뭐 먹지' 미션에서도 수정 용이성을 고려하여 텍스트 메세지의 상수화를 진행했습니다.
그때도 지금도 오버앤지니어링이라고 생각합니다만...
그때는 상수화의 이유를 명확히 생각하지 않은 채 진행한 오버엔지니어링이었다면
음, 꼭 실무 느낌을 내기 위해 오버 엔지니어링을 하지는 않으셔도 될 것 같아요. 실무 느낌이 아니어도 좋으니 동현님이 편하다고 생각하시는 방법으로 해 보셨으면 좋겠어요. 리뷰하는 입장에서도 남들이 많이 쓰는 정답같은 코드보다도 동현님만의 코드를 보고 리뷰하고 싶습니다.
저는 텍스트 메시지의 경우에는 특정 상황이 아닌 한 상수화하지 않는 편인데요, 왜냐하면...
1.39324같이 의미를 알 수 없는 매직 넘버와 달리 텍스트는 하드코딩을 해도 그 의미가 잘 드러나는 경우가 많다고 생각하기 때문이에요.- 상수화할 경우 한 곳에서 수정하기 좋을 수 있지만, 오히려 하드코딩한 그 지점으로 가서 수정하는 게 더 편할 수도 있다고 생각하고 있어요. 기능과 가장 가까운 곳에서 수정하는 거라 어떤 메시지를 수정하는지가 머릿속에 잘 들어온다고 생각해서에요.
- 고정된 메시지가 아니라 다른 변수 값에 의해 메시지 출력이 좌우되는 탬플릿 리터럴에서의 메시지 사용의 경우 그 자리에서 하드 코딩하고 변수를 넣는 편이 가독성이 더 좋은 경우가 많았던 것 같아요.
- 작성하신 텍스트 메시지의 경우 여러 컴포넌트에 걸쳐 사용되지 않은 것 같아요. 그러니까 각 메시지의 사용처가 한 곳이에요. 이 상황이라면 상수화하는 것보다 사용처인 그 컴포넌트에 그대로 하드코딩하거나, 아니면 상수를 선언하더라도 그 컴포넌트 내에 선언할 것 같아요. 이런 식으로요.
const GREET_MESSAGE = "환영합니다!";
const WelcomeMessage = () => {
return <p>{MY_MESSAGE}</p>
};
export default WelcomeMessage;제 근거를 보았을 때는 제가 텍스트 메시지를 상수화하지 않고 하드코딩해서 사용하려는 이유가 잘 드러나는 것 같나요?
2️⃣ 리뷰
간단한 애플리케이션이지만 TypeScript를 도입하셨기에 그나마 이야기할 점이 좀 더 생겼던 것 같네요!
1️⃣ 사용성
- 투두리스트에서 항목들을 추가하는 작업이 전반적으로 편했어요. 입력하고 추가 버튼 마우스로 클릭하고, 또 입력하고 마우스 클릭하고, ... 를 반복하면 불편할 수 있는데 일정을 추가하는 메뉴를 전반적으로
<form>으로 관리하셔서 추가하는 과정을 키보드(엔터 키)만으로 빠르게 다룰 수 있었어요. - 긴 일정에도 불구하고 레이아웃이 깨지지 않고
...로 생략되는 점에서 긴 이름에 대응을 하고 계신듯해 사용하기 편했습니다. 다만 그렇다 해도 감당 못할 수준으로 긴 일정을 추가하는 경우에는 로컬 스토리지와 애플리케이션이 감당하기 어려울 수도 있어보여요, 등록을 허용하는 제한 글자수를 두는 것은 어떻게 생각하시나요?
2️⃣ 요소의 기능, 의미, 역할에 맞는 HTML 태그 사용하기
요소에 맞는 HTML의 사용은 매우 기초적인 주제지만 동시에 매우 중요하기도 하죠.
<div>태그를 사용하고 다른 여러 속성들을 사용해 컴포넌트를 자세히 설명하고 기능을 붙이는 것보다 그에 맞는 적절한 태그를 사용하는 것이 효율적인 경우가 많아요. 해당 요소에 여러 의미들을 붙이기 위한 노력은 태그 다음이라고 생각해요.- 적절한 태그의 사용은 접근성에 도움이 돼요. 스크린리더의 음성에 의존해야 하는 시각장애인 사용자들, 몸이 불편해 키보드 탐색에 의존해야 하는 분 등도 원활하게 서비스를 사용할 수 있도록 해야 할 때 적절한 태그의 사용이 도움이 많이 됩니다. 스크린리더는 태그의 종류를 기반으로 더 자세한 정보를 사용자에게 음성으로 안내할 수 있고, 몇몇 키보드 탐색이 가능한 태그들을 통해 사용자가 마우스를 이용하지 않고도 버튼, 체크박스와 같이 사용자가 상호작용해야 하는 요소들을 키보드로 탐색 가능하도록 만들 수 있어요. 키보드 탐색은 일반 사용자들도 편의를 위해 사용하기도 하니 신경을 많이 써주셔야 하는 부분 중 하나라고 생각해요!
- 태그는 코드를 읽기 쉽게 만들어주기도 합니다. 태그도 코드고, 개발자는 코드를 읽으니까요. 태그 하나만 보고도 무슨 역할을 하는 요소인지 단번에 파악이 가능하죠. 기능에 큰 영향을 주지 않는
<header>,<nav>등의 시멘틱 태그들도 잘 활용하실 수 있을 것 같아요!
여기까지는 다른 리뷰이 분을 리뷰할 때와 똑같이 적었고요(강조하고 싶은 내용이 같았어요 ✅ ), 이를 기반으로 동현님의 애플리케이션을 사용할 때에는 이러한 개선되면 좋을 만한 점들이 보였습니다.
<form>태그 활용 예처럼, 현재 프로젝트에 쓰인 여러 요소들에도 용도에 맞는 태그들이 도입되었으면 하는 바램이에요.<div>가 쓰인 곳이 많은데, 보다 역할에 맞는 기능을 제공하는 태그들, 그리고 특정 의미를 지니는<nav>,<header>등의 시멘틱 태그로도- 키보드로 기능을 이용할 때 불편함을 느꼈던 것 같아요. [Tab] 키를 누르시면서, 사용자가 상호작용 해야 하는 요소들(버튼, 입력창 등등)이 잘 선택되는지 확인해 보았는데, 선택이 안 되는 요소들이 많았어요. 이것도 태그에 따라 상호작용 가능한 요소인지가 결정되니 적절한 태그 사용에 열중하시면 좋을 것 같아요!
- UI를 눈으로 확인이 어려워 음성에만 의존해야 하는 시각 장애인 사용자분들을 위해 스크린 리더가 이해할 수 있도록 여러 접근성 관련 정보들의 도입도 되면 좋을 것 같아요. 예를 들어 텍스트 버튼의 경우에는 스크린 리더가 그 텍스트를 읽어주면서 동시에 버튼이라고도 알려 주지만, 아이콘만으로 이루어진 버튼은 스크린 리더가 이게 무슨 버튼인지를 이해하기 어려운 상황이에요. 스크린 리더는 이 경우 "버튼" 이라고만 읽어줄 거에요. 그럼 음성을 듣는 입장에서는 어떤 버튼인지를 알기 어려울 것 같아요. 근데 그렇다고 해서 멀쩡한 아이콘 버튼의 디자인을 바꾸자니 이것도 맞지 않는듯한 느낌인데요, 어떻게 UI는 그대로 유지하면서 이게 특정 기능을 하는 버튼이라는 사실을 전달할 수 있을까요?
참고 자료도 올려봅니다.
- Semantic HTML의 필요성
- Wai-aria 란?
- 레진 웹 접근성 가이드라인 (1번, 4번 항목을 확인해 보시면 좋을 것 같아요, 중요하면서 준수하기도 좋은 요구사항들입니다)
3️⃣ TypeScript에서 의도에 맞는 명확한 타입 사용하기
리뷰 자체에서 할 이야기보다는 코멘트로 주로 적어 보았어요.
TypeScript에서 타입은 곧 개발자의 의도라고 생각해요. TypeScript에서 타입이 일치하지 않으면 에러를 발생시키잖아요? 다르게 말하면 개발자의 의도에 반하는 예상치 못한 값이 주어졌으니 에러를 발생시키는 것이 되겠죠. 그래서 이 의도에 맞는 명확한 타입을 사용하는 것은 중요하다고 생각해요.
사용된 타입들이 의도와 반하게 너무 느슨하지는 않은지 점검해 보실 것을 추천드리고, 의도에 반하는 값이 들어올 수 있는 환경이라면 어떻게 대처하여 타입 범위를 개발자의 의도에 맞는 타입으로 좁힐 수 있을 지도 고민해 보시면 좋겠습니다.
코멘트에도 여러 의견들을 남겨두었으니 확인해 주시면 될 것 같아요!
그럼 동현님의 의견 기다리고 있겠습니다.
| @@ -11,12 +11,18 @@ | |||
| }, | |||
| "dependencies": { | |||
| "react": "^18.3.1", | |||
| "react-dom": "^18.3.1" | |||
| "react-dom": "^18.3.1", | |||
| "react-icon": "^1.0.0", | |||
There was a problem hiding this comment.
| "react-icon": "^1.0.0", |
react-icons 라이브러리가 필요하셨던 거겠죠?
| const addTodo = (todo) => { | ||
| setTodos([...todos, todo]); | ||
| }; | ||
|
|
||
| const removeTodo = (id) => { | ||
| setTodos(todos.filter((todo) => todo.id !== id)); | ||
| }; | ||
|
|
||
| const toggleTodo = (id) => { | ||
| setTodos( | ||
| todos.map((todo) => | ||
| todo.id === id ? { ...todo, checked: !todo.checked } : todo | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
이렇게 상태 업데이트 시에 기존 값을 활용해야 하는 상황이라면, 기존 값이 최신 값임이 보장되도록 functional state update를 사용해 보시는 것은 어떨까요?
| const Header = () => { | ||
| return ( | ||
| <HeaderContainer> | ||
| <HeaderLogo src={greedySquareImage} /> |
There was a problem hiding this comment.
alt 속성을 사용하지 않으신 이유가 혹시 있으신가요? 동현님의 생각을 들어보고 싶어요. alt의 역할은 무엇일까요?
| e.preventDefault(); | ||
| if (value.trim() === "") return; | ||
| addTodo({ | ||
| id: uuidv4(), |
| const TEXTS: Texts = { | ||
| todoTemplateTitle: "Greedy Todo List", | ||
| todoListTitle: "할 일", | ||
| todoInsertPlaceholder: "할 일을 입력하세요", | ||
| todoListEmpty: "할 일이 없습니다.", | ||
| }; |
There was a problem hiding this comment.
1️⃣
Texts 인터페이스를 사용해 이 상수가 타입 추론이 되도록 하신 것 같은데, 모든 타입에 꼭 타입을 지정해 주지 않아도 기본적으로 TypeScript가 값을 토대로 타입을 추론해 줄 거에요. TypeScript가 자동으로 추론해 준 타입이 의도와 어긋나는 경우에만 타입을 지정해 보시는 것은 어떨까요?
Texts 인터페이스를 빼도 자동으로 추론이 되는 모습을 확인하실 수 있어요. 고로 제 생각에는 이 상황에서 인터페이스까지는 필요하지 않을 것으로 예상하고 있어요. 그렇지만 사용하고 계신 다른 이유가 있다면 공유해 주셔도 좋습니다 ✅
2️⃣
추가로, TEXTS 는 절대 바뀔 일이 없는 상수로써 사용하고 계시잖아요? 그렇다면 타입의 범위를 더 좁혀 의도를 드러낼 수 있을 거에요. 이 상황에서는 as const 키워드를 사용해 볼 수 있어요. JavaScript의 오브젝트는 기본적으로 변경될 수 있지만, as const 키워드 사용으로 변경이 불가능한 상수로 취급될 겁니다.
| const TEXTS: Texts = { | |
| todoTemplateTitle: "Greedy Todo List", | |
| todoListTitle: "할 일", | |
| todoInsertPlaceholder: "할 일을 입력하세요", | |
| todoListEmpty: "할 일이 없습니다.", | |
| }; | |
| const TEXTS: Texts = { | |
| todoTemplateTitle: "Greedy Todo List", | |
| todoListTitle: "할 일", | |
| todoInsertPlaceholder: "할 일을 입력하세요", | |
| todoListEmpty: "할 일이 없습니다.", | |
| } as const; |
적용 후를 확인해보세요,
string으로 추론되던 각 오브젝트의 값이 이제 특정 문자열("Greedy Todo List") 등으로 더 좁혀진 것을 확인하실 수 있어요. 이제는string일 뿐만 아니라"Greedy Todo List"가 아니면 무조건 안 된다는 의미로 좁혀진 거에요.readonly가 생겼어요. 이제TEXTS의 모든 프로퍼티의 값들은 변경되어서는 안 된다는 의미까지 더해졌습니다.
타입은 곧 개발자의 의도라고 생각해요. 적절한 상황에서 타입의 범위를 좁히고(narrowing), 필요에 따라 변경 불가능하도록(readonly) 자유도를 조절해 주시면 더 의도를 명확하게 드러내실 수 있을 거에요.
할 수 있는 것들이 많으니 타입을 자유자재로 응용해 보시기 바랍니다! 공식 문서에도 많은 정보가 있어요. 💪🏻
| <TodoInsertForm onSubmit={onSubmit}> | ||
| <TodoInsertInput | ||
| type="text" | ||
| placeholder={TEXTS.todoInsertPlaceholder} | ||
| value={value} | ||
| onChange={(e) => setValue(e.target.value)} | ||
| /> | ||
| <TodoInsertButton type="submit"> | ||
| <MdAdd /> | ||
| </TodoInsertButton> | ||
| </TodoInsertForm> |
There was a problem hiding this comment.
<form> 태그 활용 좋군요, 아래의 장점을 누릴 수 있을 것으로 보여요
- 이 코드를 보는 개발자 입장에서 이 UI가
<form>인 것이 쉽게 드러나므로 사용자의 입력값을 다루는 UI 및 기능을 담당한다는 것이 내부의 코드를 보지 않아도 추론이 될 것 같아요. - 스크린 리더에 의존하는 사용자에게도 폼이라는 정보가 잘 전달되기에 의도를 쉽게 이해할 수 있을 것 같아요.
- [Enter] 키를 누르면 폼이 전송되는
<form>의 기본 동작 을 편리하게 활용할 수 있을 것 같아요.
| } from "react-icons/md"; | ||
| import { RemoveTodo, TodoItem, ToggleTodo } from "../../types/todo"; | ||
|
|
||
| const TodoListItemContainer = styled.div<{ checked: boolean }>` |
There was a problem hiding this comment.
transient props를 사용하지 않은 이유가 있으신가요? ($checked 가 아니라 checked로 사용)
| overflow: hidden; | ||
| white-space: nowrap; | ||
| text-overflow: ellipsis; |
There was a problem hiding this comment.
아주 좋습니다 👍🏻 사용자가 매우 긴 일정을 입력하더라도 UI가 깨지지 않고 대비가 되겠군요
| export interface TodoItem { | ||
| id: string; | ||
| text: string; | ||
| checked: boolean; | ||
| } |
There was a problem hiding this comment.
잘 활용하셨습니다 👍🏻 이런 식으로 필요한 인터페이스/타입은 선언하여 활용하시길 바래요, TodoItem[] 과 같이 이미 선언하신 타입을 이용한 응용도 좋습니다
| export function loadTodos(): TodoItem[] { | ||
| try { | ||
| const savedRawTodos = localStorage.getItem("todos"); | ||
| const parsedTodos = JSON.parse(savedRawTodos ?? "[]"); | ||
| return Array.isArray(parsedTodos) ? parsedTodos : []; | ||
| } catch { | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
로컬 스토리지로부터 투두리스트를 불러오면서, 동시에 어느 정도의 검증을 해 안전한 값을 반환하도록 하는 로직이군요? 우선 로컬 스토리지 도입도 좋고, 동시에 검증도 하신 점 아주 좋다고 생각합니다 👍🏻 로컬 스토리지의 값 특성상 의도치 않게 잘못된 값이 저장될 수도 있고, 사용자가 임의로 값을 건드릴 수도 있는 영역이기 때문입니다.
여기에 더해 좀 더 꼼꼼하게 검증해 보셨으면 좋겠다고 생각해 추가 코멘트를 적어봅니다.
- 이 함수의 리턴 타입은
TodoItem[]인데, 정말TodoItem[]타입이 반환될까요? 현재parsedTodos가 거치는 검증은Array.isArray()뿐인데, 여기에서 통과했다면parseTodos의 타입은 사실상unknown[]이 아닐까요? TodoItem[]이 반환되지 않을 수도 있는 이 상황에서 TypeScript는 왜 에러를 내지 않을까요?TodoItem[]인지를 엄밀하게 검증하는 방법은 없을까요?
예를 들어 이렇게 악용을 하는 것이 가능합니다.
There was a problem hiding this comment.
추가로, localStorage에 React에서 사용되는 키 값을 저장할 경우 이 데이터는 조작을 통해 수정이 가능하기에 불러올 때 키값이 겹치는 상황도 생길 것이 예상됩니다. 이 문제는 어떻게 해결해 볼 수 있을까요? 🤔
안녕하세요 의천님(@wzrabbit )! 이번 미션 매칭된 강동현입니다.
그리디 마지막 미션을 의천님께 리뷰받게 되어 영광입니다!
그럼 이번 미션도 잘 부탁드립니다~😁
미션 목표
이번 미션의 목표는 todo어플리케이션을 만드는 것이었습니다.
미션의 요구사항은 다음과 같습니다.
TodoTemplate화면을 가운데에 정렬시켜주며, 앱 타이틀(일정관리)를 보여줌. children으로 내부 JSX를 Props로 받아와 렌더링합니다.
TodoInsert새로운 항목을 입력하고 추가할 수 있는 컴포넌트. state를 통해서 인풋의 상태를 관리합니다.
TodoListItem각 할 일 항목에 대한 정보를 보여주는 컴포넌트. Todo 객체를 props로 받아와 상태에 따라 다른 스타일의 UI를 보여줍니다.
TodoListtodo배열을 props로 받아 온 후, 여러개의 TodoListItem 컴포넌트로 변환한 후 보여줍니다.
이번 미션에 대해
이번 미션은 저번 spa 미션에 비해 공부해야 할 내용도 없고, 요구사항도 명확해서 웹페이지 구현만을 목표로 한다면 이전 미션들에 비해 난이도가 쉬운 미션이라고 느껴졌습니다.
요구사항을 꼼꼼히 안 읽고 시작해 신나게 zustand 써서 프로젝트를 다 만들고 다시 리팩터링하는등 불필요한 과정이 있었지만요..🥲
그러나 이번 미션은 다음 스텝의 미션으로 넘어가는 징검다리인 만큼, 어떠한 기능이 추가되더라도 기존 컴포넌트를 최소한으로 수정해도 되도록, 확장성과 유지보수성에 대해 생각하고 적용할 수 있는 프로젝트라고 생각하였습니다.
그래서, 이번 미션에서 다음과 같은 부분들을 신경써서 적용하였습니다.
이를 통해 구현해야 할 기능이 예상치 못한 방향으로 확장되더라도 기존 코드를 최소한만 수정하며 유지보수할 수 있도록 설계했습니다.
컴포넌트, 기능의 최소단위로의 분리
이번 미션의 요구사항이 단순해서 얼마 적용을 못했지만, 다음 사항에 적용했습니다.
ex)
loadTodos(),saveTodos(): UI와 무관한 함수는 컴포넌트와 깊게 연결되어 있지 않으면 분리.만일 다른 템플릿/페이지에서도 사용한다면 사용할 수 있도록 확장성을 고려해
loadTodos,saveTodos를 분리해 놓았습니다.타입분리, 상수화
저번 '점심 뭐 먹지' 미션에서도 수정 용이성을 고려하여 텍스트 메세지의 상수화를 진행했습니다.
그때도 지금도 오버앤지니어링이라고 생각합니다만...
그때는 상수화의 이유를 명확히 생각하지 않은 채 진행한 오버엔지니어링이었다면
지금은 가능한 한 실무 느낌을 내기 위한 고의적인 오버엔지니어링이라고 생각합니다.
또한, 스타일의 일관성을 위해 색상값 역시 상수화로 관리했습니다.
타입스크립트를 사용하며 상수화와 더불어 타입을 분리했습니다.
타입을 분리할 땐 역할 기준으로 분리하였고, 타입끼리의 참조를 최소한으로 줄이기 위해 같은 카테고리(ex) todo)의 타입별로 타입을 묶어 관리하였습니다.
props를 넘길 때는, 타입 단순화를 위해 extends를 사용하여 지역 내부에서만 사용하는 인터페이스를 지정하였습니다.
ex)
TodoTemplateProps지역 인터페이스작동 영상
todo 추가
recording.5.1.mp4
todo 취소, 삭제
recording.6.mp4
언제나 좋은 리뷰 달아주셔서 정말 감사합니다~~! 😊