-
Notifications
You must be signed in to change notification settings - Fork 50
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
[점심 뭐 먹지 미션 Step 1] 윤생(이윤성) 미션 제출합니다. #33
Conversation
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
- default 파라미터 지정으로 더 안전하게 사용 Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
Co-authored-by: afds4567 <afds4567@users.noreply.github.com>
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.
몇가지 궁금한 점 남겨보았습니다!
@@ -0,0 +1,146 @@ | |||
[ |
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.
미션 요구사항에 따른 mockData입니다.
- mockData.json의 데이터를 이용하여, 초기 데이터를 셋팅한다.
- 최소 각 카테고리 별로 3개 이상의 음식점을 mockData에 등록한다.
pipe(...funcs) { | ||
return (x, params) => { | ||
return funcs.reduce((acc, f, i) => f(acc, params[i]), x); | ||
}; | ||
} |
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.
pipe라는 개념이 있어 적용시켜 해보았는데, 기존에 있는 pipe보다는 각색한 느낌이 있습니다.
pipe(f1, f2, ...)(x, [p1, p2, p3 ,...])
fn : 순서대로 호출되는 함수 : fn(fn-1( ... f1(x, p1), pn-1), pn)
x : 함수 f가 조작할 데이터
[p1, p2, ..., pn] : 함수 f에 전달할 파라미터
ex)
pipe(f1, f2)(x, p1, p2) = f2(f1(x, p1), p2)
확실히 재사용성이 떨어지는 느낌인데.. (적용할 수 있는 함수f의 형식이 제한적임) 어떻게 개선해볼수 있을까요?
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.
여러 함수들을 통한 연산을 연속적으로 수행할 때 사용하는 개념인데, 어떤 식으로 재사용성이 떨어진다고 생각이 들었을까요?? pipe 를 쓰지 않으면 복잡하게 구현될 내용들이 pipe 를 쓰면서 가독성은 조금 떨어지더라도 짧고 간결하게 표현하기에 좋은거 같아요!
제가 생각하기에는 pipe 란 함수는 특정 파일에 속해 있기보다는 유틸성 함수가 더 어울릴거 같아요~
이렇게 새롭게 배운 함수를 사용해보는건 좋은거 같아요 👍
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.
제가 구현한 파이프는 전달받을 데이터 x, 그 이외 파라미터 하나를 받을 수 있는 함수만 사용가능한, (x, p) => x
만 받을 수 있어 매개변수 숫자를 달리 쓰고 싶을 때는 적용하지 못해 그렇게 적었습니다!
@@ -0,0 +1,298 @@ | |||
* { |
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.
마크업은 제공한 마크업을 그대로 사용하였습니다!
이번 미션에서는 리액트에서 집중하라는 의미에서 CSS는 이전의 코드에서 가져와도 된다고 하셔서 그대로 적용하였습니다.
{this.props.options.map((option) => ( | ||
<option key={option.value} value={option.value}> | ||
{option.textContent} | ||
</option> | ||
))} |
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.
네 좋은거 같아요 👍
셀렉트 버튼은 충분히 재사용성이 높아서 이렇게 프롭스로 받아서 보여주면 좋겠네요.
추가로 좀더 재사용성을 높이기 위해, 옵션을 클릭했을때 실행되는 이벤트도 프롭스로 받아봐서 설계해보면 어떨까요?
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.
<select>
태그에 이벤트를 등록해서 사용하는 걸로 알고 있어서 onChange 이벤트를 props로 받아서 처리했어요!
어떤 부분이 이해가 안되었는지 다음 피알때 지금처럼 코드 아래다가 표시해주면 좋을거 같아요~ |
제가 생각한 기준은 외부에서 받아온 데이터라도 변하지 않는 데이터라면 굳이 state 로 관리할 필요 없다고 생각합니다. 왜냐하면 변하지도 않는 데이터에 변화할수 있는 형태를 열어주는것이 휴먼에러와 연결된다고 생각합니다. 또 공식문서와는 다른 생각을 가지고 있는데, 반대로 부모로 부터 받은 데이터도 상태가 변하고 뷰에 영향을 주는경우 state로 관리되어야한다고 생각합니다. |
이 부분은 이펙티브 타입스크립트에도 명시된 내용인데, 사실 둘의 차이점은 크게 없습니다! 중요한건 하나로 통일해서 써야하는거 같아요. 책에서 나온 내용을 토대로 보면 타입간의 합성등을 자주 쓰면 성능상, 편의상 또 굳이 차이점을 찾아보면, 서로의 선언방법이나 확장하는 방법등 사용법이 다르지만, 결국 대부분의 키 타입 피쳐는 둘다 제공해서 통일되는게 제일 중요할거같아요~ |
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.
안녕하세요 윤생~
처음 뵙겠습니다 🙂 이번 미션 윤생의 리뷰어를 맡게된 케빈입니다.
윤생의 코드는 처음본거 같은데, 구조도 잘잡혀있고, 새로운 문법도 여러가지 시도해보신거 같아서 재밌게 코드를 읽었던거 같아요 💯
코드하단에 궁금한 부분이나 고민했던 부분을 따로 적어주셔서 내용을 쉽게 파악할 수 있었던거 같아요~
남겨드린 코멘트 확인해주세요 💪 다음 미션에서 만날게요
type AppState = { | ||
filterOptions: FilterOption; | ||
isOpenDrawer: boolean; | ||
drawerSelectId: number; | ||
}; |
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.
type 은 다른 파일에서 공통적으로 사용할 수 도있고, 확장해서 사용할수도 있어서 따로 빼놓는건 어떨까요?
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 컴포넌트의 상태를 명시해주는 건 오히려 외부 파일로 분리하면 App.tsx
를 읽을 때 번갈아서 봐야될 것 같다는 생각이 들어 함께 넣어 두었습니다. 또한 AppState
는 App의 state가 가질 타입이라고 생각이 들어 다른 곳에서는 사용하지 않을 것 같아서 같은 파일에 넣어두었습니다! 이것에 대해 케빈은 어떻게 생각하시나요??
type RestaurantDetailDrawerState = { | ||
restaurant: Omit<Restaurant, 'id'>; | ||
}; | ||
|
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.
👍 네이밍이 엄청 자세하게 잘 되어 있네요 👍
<button | ||
type="button" | ||
className="button button--secondary text-caption" | ||
onClick={() => this.props.onToggleDrawer()} |
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.
onClick={() => this.props.onToggleDrawer()} | |
onClick={this.props.onToggleDrawer} |
로도 사용가능할거 같아요 👍
<h3 className="restaurant__name text-subtitle"> | ||
{this.props.restaurant.title} | ||
</h3> | ||
<span className="restaurant__distance text-body"> | ||
캠퍼스로부터 {this.props.restaurant.distance}분 내 | ||
</span> | ||
<p className="restaurant__description text-body"> | ||
{this.props.restaurant.description} | ||
</p> |
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.
html 태그를 굉장히 잘 사용하시네요 👍
윤생이 정한 span 태그와 p 태그 사용 기준이 있을까요?
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.
사실 이것도 마크업을 그대로 가져온겁니다! 하지만 제가 가지고 있는 span 태그와 p 태그 사용 기준은 span은 짧은 내용(단순 정보 => 한줄 정보 등등)을 담을 때 주로 사용하고 p는 단순 정보만 포함하지 않는 어떠한 주제에 대해 내용을 구성하는 녀석들일 때 사용하고 있습니다!
|
||
type StateType = { | ||
restaurantList: Omit<Restaurant, 'link'>[]; | ||
}; |
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.
StateType 보다는 좀더 명확한 네이밍을 사용하면 좋을거 같아요~!
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.
헉! 그러네요 ㅠ 리팩터링할때 주의깊게 봐야겠어요
pipe(...funcs) { | ||
return (x, params) => { | ||
return funcs.reduce((acc, f, i) => f(acc, params[i]), x); | ||
}; | ||
} |
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.
여러 함수들을 통한 연산을 연속적으로 수행할 때 사용하는 개념인데, 어떤 식으로 재사용성이 떨어진다고 생각이 들었을까요?? pipe 를 쓰지 않으면 복잡하게 구현될 내용들이 pipe 를 쓰면서 가독성은 조금 떨어지더라도 짧고 간결하게 표현하기에 좋은거 같아요!
제가 생각하기에는 pipe 란 함수는 특정 파일에 속해 있기보다는 유틸성 함수가 더 어울릴거 같아요~
이렇게 새롭게 배운 함수를 사용해보는건 좋은거 같아요 👍
restaurant: Omit<Restaurant, 'id'>; | ||
}; | ||
|
||
class RestaurantDetailDrawer extends React.Component< |
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.
이번 미션에서 요구사항에 있어서 class 를 사용하신걸로 알고있는데,
function 으로 바뀌면 코드가 어떻게 바뀔지, 생태주기는 어떻게 될지, 서로의 장단점은 뭔지 한번 생각해봐도 좋을거 같아요~
{this.props.options.map((option) => ( | ||
<option key={option.value} value={option.value}> | ||
{option.textContent} | ||
</option> | ||
))} |
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.
네 좋은거 같아요 👍
셀렉트 버튼은 충분히 재사용성이 높아서 이렇게 프롭스로 받아서 보여주면 좋겠네요.
추가로 좀더 재사용성을 높이기 위해, 옵션을 클릭했을때 실행되는 이벤트도 프롭스로 받아봐서 설계해보면 어떨까요?
안녕하세요 케빈! 한 주간 잘 부탁드립니다!
구현 방향
1. 동작 가능한 가장 작은 버전부터 만들기. 단, 핵심을 포함하게
그래서 저는 이번 앱의 타이틀이 "점심 뭐먹지" 인 만큼, 사용자에게 음식점을 띄워주는 기능이 먼저겠다 생각이 들었습니다. 그래서 각 음식점의 목록 정보를 보여주는 기능을 먼저 구현하였습니다.
사실 상세 정보창(
Drawer
)를 띄워주는게 우선이겠다고도 생각해보았는데, 그것 보다는 사용자가 궁금한 건 어떤 음식점들이 있는지 목록을 보는 것이 가장 주요한 핵심 기능이라 생각해 먼저 구현하였습니다.2. 각 컴포넌트 별 책임 분리
그리고 설계 단계에서, 각 컴포넌트 계층이 어떤 책임을 가져야 할지 고민해보았습니다. 그래서 간단히 요구사항에 정리해보았고, 그 책임에 따라 구현하려고 해보았습니다.
3. 타입스크립트를 리액트에 적용해보기
타입스크립트를 리액트에 적용해보는 건 처음인데요..(물론 TS자체도 우테코에서 처음 다뤄보았습니다) 미션 시간 상 다소 이해가 안가도 일단 적용하고 넘어간 부분이 많아, 세세한 피드백 부탁드립니다!
4. 컴포넌트 재사용성 고민
컴포넌트를 재사용하기 위해 합성을 사용하였습니다. 예로
Drawer
, 같은 경우props
로 받은 상태에 따라 열고 닫히는 기능만 있도록 추상화하고, 합성을 사용해 구체화 하였습니다.대략적인 구현 방향은 이정도입니다!
궁금한 점
Q. state에 관하여
state
로 관리해야 될까요? 일단은render
에서 띄워주기 위해state
로 가지고 있어야 된다고는 생각하는데, 무언가 맞지 않는 것 같네요. 특히 리액트 공식 문서에서 참고한 내용으로는, 어플리케이션 안에서는 변하지 않는 데이터지만 외부에서 받아오는 데이터라 변하기 때문에state
로 관리해야할까요? (아래는 공식문서에서 발췌한 내용입니다.)Q. state와 props 타입을 지정해줄 때 interface를 사용해야 할까요? type을 사용해야 할까요?
type
과interface
의 차이점이 잘 느껴지지는 않아서요. 어떻게 고민해보면 좋을까요?이상입니다! 감사합니다~!! 추가적으로 궁금한 점은 코드에 달아두겠습니다.
etc