-
Notifications
You must be signed in to change notification settings - Fork 0
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
매치 상세내역 firebase 연동 #39
Conversation
Visit the preview URL for this PR (updated for commit 4f6b874): https://dailyfield-a845d--pr39-getmatch-st2uwog8.web.app (expires Sun, 07 Feb 2021 13:16:21 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
src/components/MatchDetail.tsx
Outdated
const [match, setMatch] = useState<Match | null>(null); | ||
|
||
const getMatch = useCallback(async () => { | ||
const matchId = history.location.pathname.split("/")[2]; |
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.
컴포넌트 안에서 history location 읽으면 말끔하지 않아. useParameter써봐.
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.
useParameter 쓰면 useCallback 필요 없나봐
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.
useParameter 사용 완료
src/components/MatchDetail.tsx
Outdated
|
||
useEffect(() => { | ||
getMatch(); | ||
}, [getMatch]); |
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.
[getMatch]
맞지 않을것 같아. 왜냐면 getMatch이 함수야. 값이 안 바꿔. 뭘 필요해?
https://reactjs.org/docs/hooks-reference.html#conditionally-firing-an-effect
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.
src/components/MatchDetail.tsx
Line 17:6: React Hook useEffect has a missing dependency: 'getMatch'. Either include it or remove the dependency array react-hooks/exhaustive-deps
때문에 deploy 자꾸 실패해... ㅠㅠ
}), | ||
}); | ||
}); | ||
|
||
test("renders MatchDetail", async () => { | ||
render(<MatchDetail />); |
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.
가짜 useParameter 값을 줘야해. 그리고 await db.collection("matches").doc(matchId).get();
의 matchId 확인 해줘.
expect(useParameter의 준 id).toBe(.doc이 쓴matchId)
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.
그런데 그 테스트 만들기 너무 어려우면 나중에 #43 처리 할때 해도 돼.
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.
#43 처리할때 같이 처리할게!
src/components/MatchDetail.tsx
Outdated
|
||
useEffect(() => { | ||
getMatch(); | ||
}, [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.
응. getMatch을 useEffect안에 옮겨줘. 문서 다시 한번 읽어봐
usually you’ll want to declare functions needed by an effect inside of it.
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.
useEffect 안에 옮겨서 해결했는데 나중에 한번더 설명해줘! ㅎㅎ
matchDetail에서 dummy data -> firebase 실제 데이터 연동으로 변경