Seal 1주차 #28
Merged
Seal 1주차 #28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 상수화 - 유틸함수 설정 - 함수명 변경
- 상수화 - 함수 분리 - 함수명 변경
- controller 제거 - view 제거
sunhwa508
reviewed
Mar 16, 2022
sunhwa508
reviewed
Mar 16, 2022
dhrod0325
reviewed
Mar 16, 2022
yechukim
reviewed
Mar 16, 2022
InSeong-So
reviewed
Mar 17, 2022
packages/seal/src/views/MainView.js
Outdated
Comment on lines
88
to
105
| switch (tab) { | ||
| case 'saved': { | ||
| saved.style.display = 'block'; | ||
| container.style.display = 'none'; | ||
|
|
||
| this.renderBookmark(); | ||
| break; | ||
| } | ||
| case 'explore': { | ||
| saved.style.display = 'none'; | ||
| container.style.display = 'block'; | ||
|
|
||
| this.globalIndex = 0; | ||
| this.loadMore(); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
switch-case 문보다 괜찮은 방법은 없을까요? 2개 조건밖에 없는데 과도한 느낌이 듭니다. if문 하나로도 처리할 수 있을 것 같아요✍
There was a problem hiding this comment.
저도 고민 했는데, 확실히 탭 별로 구분하기 위해서 코드가 길어지더라도 switch-case 문을 넣었습니다. 단순 if문으로 분기 처리를 해도 가독성을 보장할 수 있을지는 조금 의문이긴 합니다..!!
dhrod0325
reviewed
Mar 17, 2022
areumsheep
reviewed
Mar 17, 2022
- 이전 함수 이름이 아닌, 변경된 이름으로 변경
- 의도를 파악하기 쉽게 하도록 애매한 변수 명(result) 변경
InSeong-So
reviewed
Mar 20, 2022
Comment on lines
+61
to
+70
| loadMoreWithDebounce() { | ||
| return debounce(() => { | ||
| const container = $('.container'); | ||
| const pinList = []; | ||
| for (let i = MAIN_EXPLORE_PAGE_PER_AMOUNT; i > 0; i--) { | ||
| pinList.push(this.createPin()); | ||
| } | ||
| container.innerHTML += pinList.join(''); | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
디바운스 함수를 클로저로 처리했는데, 스코프를 숨겨야 하거나 로컬 변수를 유지할 목적이 아닌 것 같은데... 의도를 알 수 있을까요?👀
There was a problem hiding this comment.
클래스 내부의 메소드 컨벤션이 일반 함수 형태로 되어 있는데, debounce를 wrapping하게 되면 화살표 함수로 만들어야 되는 것 같더라고요.. 그래서 통일성 있게 만들고자 좀 억지스러운 모양이 나오긴 했습니다.
|
@InSeong-So 하나의 pr이 너무 길어지는 것 같아요! 이제 머지하고 다음 pr로 이동할까 하는데, 어떻게 생각하시나요? |
|
이렇게 열심히 PR하고 코드 리뷰를 할 줄 몰랐는데... 다음 세션 때는 방법을 좀 개선해서 공지해야겠네요! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔎 개요
📝 진행상황
refactor : mvp 패턴 간소화라는 메세지가 있는데, 이 부분은 실수입니다..!🖼 스크린샷
💬 의견