-
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
[FE- TEAM12] [Junami, Eamon] PR2 #71
Conversation
Fe/0430 재고 ctrl
-atomic 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.
- hook의 사용 관점에 대해 리뷰드렸어요.
- forwardRef 와 useImperativeHandle을 잘 이해하고 쓰신 것 같아요. 리엑트 lib 컴포넌트들이 ref를 제공해서 제어권을 밖으로 넘겨주는 방법인데, 특정 상황에는 잘 쓰입니다. 👍 👍
- hook 디자인할 때 https://github.com/rehooks/awesome-react-hooks 요런 링크도 참고해보세요.
- https://use-http.com/#/?id=basic-usage-auto-managed-state 이런 hook의 인터페이스만 동일하게 해두고 내부구현은 어떻게든 하는 식으로 참고해보면 어떨까요?
useEffect(() => { | ||
if (trasitionValue === 'none') setTransitionValue(transitionDefault); // eslint-disable-next-line | ||
}, [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.
p3;
dependency 배열에 [x]
만 들어가 있는데, lint disable 까지 하신걸 보니 특정 동작을 의도하신 것 같아요.
그러나 이렇게 사용하기 시작하면 코드를 작성한 분 이외에는 이 코드가 어떤 원리로 돌아가는지 파악하기 어려울 것 같습니다.
async function fetchUrl() { | ||
if (url === null || url === undefined) return; | ||
try { | ||
const res = await axios({ url, method, code }); | ||
setData(res.data); | ||
} catch (error) { | ||
if (error.response.status === 400) { | ||
setData(400); | ||
console.error('요청주소에 문제가 있어요😯', error.response.status); | ||
} | ||
} finally { | ||
setLoading(false); | ||
} | ||
} | ||
useEffect(() => { | ||
fetchUrl(); | ||
return () => { | ||
setData([]); | ||
setLoading(true); | ||
}; // eslint-disable-next-line | ||
}, [url]); | ||
|
||
return [data, loading]; |
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.
p3;
요런식으로 하는게 더 원하는 바를 잘 표현한 것 같네요.
const fetchUrl = useCallback(() => {..}, [url]);
useEffect(() => {...}, [fetchUrl])
const basicUrl = process.env.REACT_APP_API_URL + 'best/'; | ||
const [bestDishMenu, bestDishLoading] = useFetch(basicUrl, 'get'); |
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.
요런식의 hook으로 바꾸면 좀 더 선언적으로 쓸 수 있습니다.
Hook을 재사용 관점뿐만 아니라, 관련 로직을 하나로 응집한다는 개념으로 사용해서, 특정 컴포넌트에서만 사용하는 hook을 만들어 쓰면 좋습니다. 그렇게 쓰면 뷰(jsx)와 로직이 분리되는 효과도 있습니다.
const useBestDishMenu = (basicUrl) => {
const [bestDishMenu, bestDishLoading] = useFetch(basicUrl, 'get');
return { besDishMenu, bestDishLoading }
}
import { VscChevronLeft, VscChevronRight } from 'react-icons/vsc'; | ||
|
||
function CategoryRender({ title, url }) { | ||
const button = useRef(); |
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.
p5 요런 변수명이 더 좋겠네요.
const button = useRef(); | |
const buttonRef = useRef(); |
const transitionDefault = `all ${effect} ${duration}`; | ||
const panelWidth = width / count; //320 | ||
|
||
let block = []; | ||
for (let i = 0; i < children.length; i += count) { | ||
block.push(children.slice(i, i + count)); | ||
} | ||
const lastBlockCount = block[block.length - 1].length; | ||
const startX = -width - lastBlockCount * panelWidth; | ||
|
||
const [x, setX] = useState(startX); | ||
const [moving, setMoving] = useState(false); | ||
const [blockNumber, setBlockNumber] = useState(1); | ||
const [trasitionValue, setTransitionValue] = useState(transitionDefault); | ||
|
||
const onMove = (direction) => { | ||
if (moving) return; | ||
if (direction === 1) { | ||
setX((prevX) => | ||
blockNumber === block.length | ||
? prevX + direction * lastBlockCount * panelWidth | ||
: prevX + direction * width, | ||
); | ||
setBlockNumber((prevCnt) => | ||
blockNumber === 1 ? block.length : --prevCnt, | ||
); | ||
} else { | ||
setX((prevX) => | ||
blockNumber === block.length - 1 | ||
? prevX + direction * lastBlockCount * panelWidth | ||
: prevX + direction * width, | ||
); | ||
setBlockNumber((prevCnt) => | ||
blockNumber === block.length ? 1 : ++prevCnt, | ||
); | ||
} | ||
setMoving(true); | ||
}; |
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.
이런 로직을 묶어서 hook으로 분리해도 좋겠네요.vanila js에서 관련성 높은 부분을 묶어서 함수로 표현하는 것처럼,
리엑트에서는 hook으로 표현할 수 있습니다. hook을 너무 재사용 관점으로 생각하지 않으셔도 됩니다. 로직을 묶어서 표현하는 표현양식이라고 생각하면 좋을 것 같아요.
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.
코드 들여쓰기가 너무 깊으니 vscode설정을 해보세요.
for (let i = 0; i < children.length; i += count) { | ||
block.push(children.slice(i, i + count)); | ||
} |
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.
고차함수를 사용해보시죠.
const onMove = (direction) => { | ||
if (moving) return; | ||
if (direction === 1) { | ||
setX((prevX) => | ||
blockNumber === block.length | ||
? prevX + direction * lastBlockCount * panelWidth | ||
: prevX + direction * width, | ||
); | ||
setBlockNumber((prevCnt) => | ||
blockNumber === 1 ? block.length : --prevCnt, | ||
); | ||
} else { | ||
setX((prevX) => | ||
blockNumber === block.length - 1 | ||
? prevX + direction * lastBlockCount * panelWidth | ||
: prevX + direction * width, | ||
); | ||
setBlockNumber((prevCnt) => | ||
blockNumber === block.length ? 1 : ++prevCnt, | ||
); | ||
} | ||
setMoving(true); | ||
}; |
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.
코드 들여쓰기를 에디터에서 설정해보세요.
너무 들여쓰기 크기가 크네요.
|
||
const onMove = (direction) => { | ||
if (moving) return; | ||
if (direction === 1) { |
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 이라는 표현은 방향을 의미하지 않아서 별로같아요.
token !== null && setLoginState(true); | ||
|
||
const getUserInfo = async () => { | ||
const { data } = await axios.get('https://api.github.com/user', { |
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.
axios 와 같은 라이브러리를 만들면 좋겠어요.
width={1280} | ||
height={242} | ||
count={4} | ||
duration={'.5s'} | ||
ref={button} | ||
effect={'ease-in-out'} |
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.
옵션이 많아서 좋네요.
(장문 주의😈)
🔢 추가구현사항 : 금요일 데모 내용 + 로그인 기능 추가 구현
(로그인정보를 얻어와 유저정보(깃 프로필/네임) 표기)
👉 리드미
주나미
-useFetch 커스텀 훅을 쓰긴했지만 상황에 따라서는 직접 fetch, axios를 다양하게 사용하였습니다
useFetch 하나로 해결하지 못한게 아쉬웠는데, 아무래도 파라미터 전달하는 방식을 수정하면 해결될수있었을것 같은데
잘 모르겠습니다. 어떻게 고치면 여러 인자들을 다루는 fetch훅을 만들수있을지 고민입니다.
또 지금은 useFetch이후의 error처리를 따로 해야하는데 한번에 에러까지 다루는 방법을 적용해보고 싶습니다.
그런데 작업을 하면서 재사용되는 컴포넌트들이 생기는걸 확인하고 작업을 하면서 컴포넌트를 만들거나, 또는 만들고나서 분리하는 작업이 필요했는데 사전에 충분한 분석을 하고 시작해야겠다고 느꼈습니다.
2주간 사이좋게 함께한 이몬과 백엔드 이노, mj 감사합니다~ : )
이몬
"공부한다" 라는 느낌보다는 "미션을 구현한다" 라는 마음가짐으로 2주를 보냈던 것 같다. 앞으로 남은 기간동안 미션을 하더라도 하루에 하나씩 알아 가는 것으로 구현에 급급해하지 말자. 지금은 일하는 것이 아니라 배우는 기간이다. 하나라도 제대로 알고 넘어가자