Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #2
base: main
Are you sure you want to change the base?
[8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #2
Changes from all commits
37ac031
46c2252
ba6557b
17df315
e398fa5
1c6fc3a
51a666e
0ff32e7
245951e
05b5ecc
e209f2b
96c087f
2e255fc
e26bbb5
0a3d138
11e904a
c7c7541
e5ad353
1765bc8
3db4eae
70f9469
d38022a
20ff127
a1339b7
f5f40fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
저는 제시된
activationLike
를 그대로 사용했는데, 어차피 likeUsers를 전달해주니, 굳이 위에서 체크할 필요가 없었군요..꼼꼼하게 체크하신게 느껴집니다 👍
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.
document.getElementById로 ID를 하드코딩하는 것보다 event를 prop으로 받아서 event.target으로 접근하는 방법이 더 안정적일 것 같은데 어떻게 생각히사는지요?!
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.
normalizeVNode.js
의vNode.type === "function"
를 통해컴포넌트 함수는 다 처리된 후 createElement로 전달될 것 같은데 혹시 한번 더 체크하시는 이유가 있으신지, 예외케이스가 있으셨는지 궁금합니다!
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.
여기서 vNode가 배열일 경우
createElement
가 아니라makeElement
로 처리해주셨는데, 배열 요소에vNode === "string" || vNode === "boolean"
요런 케이스에 해당하는 요소가 올 경우에 오류가 발생하지 않으셨는지 궁금합니다!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.
개인적으로는
replacePropIsClass
로 별도의 함수로 뺐다면, 윗줄에서 한 번 실행하고 반환값을 받아서prop
을 인자로 넘겨주는 쪽이 가독성이 더 좋을 것 같아요!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.
updateAttributes 함수를 보면 기존 코드는 모든 속성을 매번 설정하고 있어서 DOM 요소에 이미 설정된 속성이라도 변경 없이도 불필요하게 다시 설정되고 있네요 혹시 맞나용??
제가 개선해보면서 addEvent 함수를 호출하기 전에 이벤트 리스너 속성이 변경되었는지 확인하는 로직까지 한번 추가 해봤는데 요렇게 추가로 해보시는건 어떤가요?? 조금 더 고도화 하면 좋을 것 같습니다 :) 이 함수의 역할이 잘 보이게 간결하게 잘 작성하신 것 같아요!! 좋습니다 👍
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.
오 코드 개선까지 감사합니다.
여기서 궁금한게,
props[key] !== prevProps[key]
코드에서 핸들러 함수가 상이한 것을 체크할 수 있나요??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.
eventManager에서 중복된 이벤트 이슈를 어떻게 해결해야할지 감이 안잡혔는데, 준만님 코드를 보고 많이 배웠습니다!👍👍👍