-
Notifications
You must be signed in to change notification settings - Fork 0
과제제출합니다 #57
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
base: main
Are you sure you want to change the base?
과제제출합니다 #57
Conversation
sangheon-kim
left a comment
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 target = useRef(null) | ||
|
|
||
| return <></> | ||
| } |
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.
이건 무슨 파일인가요
| //<NavLink to={`/movies/${movie.imdbID}`}></NavLink> | ||
|
|
||
| export default MovieList | ||
| //props 사용하기 |
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.
movies가 안들어올 수도 있다는 가정하에 옵셔널 처리도 안되어있어서 런타임 에러날 것 같아요
| type="text" | ||
| value={props.value} | ||
| onKeyDown={e => { | ||
| if (e.keyCode === 13) props.setSearchValue(e.target.value) |
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.
onKeyDown을 쓰지말고
Form 태그로 감싸서onSubmit으로 받으면 되지않나요?
| import React from 'react' | ||
| import ReactDOM from 'react-dom/client' | ||
| import { RouterProvider } from 'react-router-dom' | ||
| import router from '~/routes' |
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 [searchValue, setSearchValue] = useState('') | ||
| const [show, hide] = useState({ display: 'block' }) | ||
|
|
||
| const getMovieRequest = async (searchValue, page = 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.
api는 별도로 분리해주세요
| } | ||
|
|
||
| useEffect(() => { | ||
| getMovieRequest(searchValue) |
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 json = await res.json() | ||
| console.log(json) | ||
|
|
||
| if (json.Response === '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.
흠... Response 타입을 스트링으로 내려준게 의아하네요
fetch 자체적으로 감지할 수 있는게 있을건데요
| <h3>Genre</h3> | ||
| <p>{movieInfo.Genre}</p> | ||
| </div> | ||
| </div> |
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.
어느것 하나 옵셔널체이닝으로 예외처리가 안되어있네요 API에러나면 클라이언트 뻗을 것 같아요
과제제출합니다