프론트 리뷰 시작합니다 #4
프론트 리뷰 시작합니다 #4
Conversation
There was a problem hiding this comment.
전체적으로 하나의 함수는 하나의 역할만 하도록 작성하려고 노력한것이 보이구요
구조자체를 잘 나눈것같고 네이밍에 연관성 있는것들끼리 일관성을 둔것도 매우 잘한것같습니다.
테스트 코드를 작성하는 습관도 매우 좋은것같습니다.
몇가지 아쉬운점을 말하자면
- 스타일 코드 작성에 대한 일관성
- 줄바꿈이나 코드 컨벤션과 관련된 일관성
- 기능이나 api 에 대한 테스트가 아닌 ui 에 테스트가 집중되었다는점
- 반복 사용되는 태그들에 대한 컴포넌트의 부재
스타일 컴포넌트같은경우 어떻게 활용하느냐에 따라 매우 잘 작성되어질수도있고 오히려 방해요소가 될수도있는 까다롭고 난이도가 있는 스타일 작성법인것같습니다. 해당 부분에 대한 고민이 너무 깊고 또 가이드라인을 정하고 일관성있게 사용하는것이 어렵다면 scss 로 작성해보는것도 고려해볼 수 있을것같습니다.
기능 동작에는 무리가 없지만 같이 협업할 동료로서 코드를 본다면 해석하기 난해하거나 시간이 필요한 부분들이 보여집니다. 컨벤션을 유지할 수 있는 도구를 활용해 보시기를 추천드립니다.
| <a onClick={deleteButtonHandle}>삭제</a> | ||
| <a className="Save-Button" onClick={saveButtonHandler(startDate, endDate)}> | ||
| 저장 | ||
| </a> |
There was a problem hiding this comment.
a 태그는 보통 다른 페이지로 이동하는 하이퍼링크로 사용할때 활용합니다.
페이지 이동이 아닌 클릭 이벤트에 의한 삭제나 저장 이 목적이라면 button 태그가 더 적합한 요소일것같습니다.😊
| @@ -0,0 +1,14 @@ | |||
| import styled from 'styled-components'; | |||
|
|
|||
| export const Modal = styled.div``; | |||
There was a problem hiding this comment.
스타일 작성을 위해 미리 만들어 두신거겠죠?
만약 사용하지 않는 코드라면 제거하는것이 더 좋을것같습니다~
| <Style.ItemNameWrapper>{state[2]}</Style.ItemNameWrapper> | ||
| <Style.ControllButtonWrapper> | ||
| <button | ||
| className={state[0] === 0 ? 'isNonClickable' : ''} |
There was a problem hiding this comment.
className 작성과 관련해서 형식을 통일해주시면 좋을것같습니다.
카멜이든 _ 형식이던 어떤걸써도 사실 상관은없는데 나중에 현업에서는 코드컨벤션에 가이드라인을두고 통일성을 두고 작성하기때문에 한가지 방식을 유지하거나 만약 그게 어렵다면 lint 나 도구를 통해서 강제하는 방법도 있습니다
|
|
||
| import Slider from '@material-ui/core/Slider'; | ||
|
|
||
| const PriceRangeSlider = ({ value, minPrice, maxPrice, rangeSlided, saveButtonHandle, deleteButtonHandle }) => { |
There was a problem hiding this comment.
매개 변수들을 넘겨받을때 라인수가 길어지더라도 한라인당 변수 하나씩 작성되도록 하면 가독성이 더 좋아질것같습니다.
이건 필수는 아니고 권장사항이므로 참고만 한번 해주세요~
{
value,
minPrice,
maxPrice,
rangeSlided,
saveButtonHandle,
}| personnelControlled={([adult, children, infant]) => { | ||
| const guest = adult + children + infant; | ||
| const buttonName = | ||
| guest === 0 ? '인원' : `게스트 ${guest}명${infant === 0 ? '' : `, 유아 ${infant}명`}`; |
There was a problem hiding this comment.
복잡한 조건들이 얽혀있다면 함수로 만들어서 활용할 수 있을것같습니다.
삼항연산자는 딱 한번의 사용으로 한줄로 끝날때 제일 깔끔하게 사용가능한것같습니다.
삼항 연산자내에서 삼항연산자가 중첩으로 포함된다면 정확한 조건에 대한 상태를 코드로 읽어 나가는데 협업시에 좀 어려움이 있을것같습니다.
| getByText('날짜'); | ||
| getByText('인원'); | ||
| getByText('숙소 유형'); | ||
| getByText('가격'); | ||
| getByText('필터 추가하기'); | ||
| }); |
There was a problem hiding this comment.
getByText 로 텍스트를 선택하는 것만 작성해주셨는데 expect로 확인하는것이 테스트 실패 했을경우 명확하게 실패 사유를 확인 가능하니 it 구문 하나당 expect 가 포함되어서 테스트가 수행될 수 있도록 하면 좋을것같습니다.
| <input id="email" type="text" placeholder="Email" onChange={e => setEmail(e.target.value)} /> | ||
| <input | ||
| id="password" | ||
| type="password" | ||
| placeholder="Password" | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> |
There was a problem hiding this comment.
전체적으로 라인 줄바꿈과 관련해서 일관성이 없는 부분들이 있는것같습니다. 일관성 유지가 어렵다면 lint 를 활용해보시길 추천합니다.
| }, [data, loading, error]); | ||
|
|
||
| const register = () => { | ||
| registerUser({ variables: { data: { email, password, name } } }); |
There was a problem hiding this comment.
여러번 중첩되는 객체를 활용할때도 가독성을 위해 줄바꿈을 해주시면 훨씬 보기 좋을것같습니다👍
| switch (type) { | ||
| case 'reset': { | ||
| return initialSearchOptionState; | ||
| } | ||
| case 'setDateFilter': { | ||
| return { ...state, searchOptions: { ...state.searchOptions, date: payload } }; | ||
| } | ||
| case 'setPersonnelFilter': { | ||
| return { ...state, searchOptions: { ...state.searchOptions, personnel: payload } }; | ||
| } | ||
| case 'setPriceFilter': { | ||
| return { ...state, searchOptions: { ...state.searchOptions, price: payload } }; | ||
| } | ||
| default: { | ||
| throw new Error(`unexpected action.type: ${type}`); |
There was a problem hiding this comment.
{
...state,
searchOptions: {
...state.SearchOptions,
[type]: payload
}
}이렇게 활용해본다면 switch 문으로 작성하지않고 처리할 수 있을것같은데 참고해주시면 좋을것같습니다.
No description provided.