-
Notifications
You must be signed in to change notification settings - Fork 89
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
[1단계 - 음식점 목록] - 초코(강다빈) 미션 제출합니다. #127
Conversation
Co-Authored-By: 00kang <[email protected]>
- eslint - prettier - tsconfig(경로 별칭) - package.json(typescript-eslint 설치) Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
- 페이지가 로드됐을 때 - 필터를 변경할 때 Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
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.
안녕하세요~ 초코
초코의 우려와는 달리 전체적으로 구조도 괜찮고 불필요한 오버엔지니어링도 없어도 너무 보기 좋았습니다. 앞으로 2번째 미션에서는 이를 활용해야할텐데 어떻게 코드를 작성하면 서비스가 확장할때도 유연하게 대응할 수 있을지를 고민해보면 좋을거같아요 👍
조금 아쉬웠던건 class명이나 id 명에서 일관성이 없는거 같은데 이부분도 같이 확인해주시면 좋을거 같아요!
미션 완료하느라 고생많으셨습니다 👍 💯
__test__/RestaurantManager.test.js
Outdated
import RestaurantManager from '../src/domain/RestaurantManager'; | ||
|
||
describe('RestaurantManager 기능 테스트', () => { | ||
const RESTARANT_A = { |
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.
헉 오타 주의하겠습니다!
src/domain/RestaurantManager.ts
Outdated
#sortByName(restaurants: Restaurant[]): Restaurant[] { | ||
return restaurants.sort((a, b) => { | ||
if (a.name > b.name) return 1; | ||
if (a.name < b.name) return -1; | ||
return 0; | ||
}); | ||
} |
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.
어떤 배열을 다루는 함수를 작성할때는 직접 원본 배열을 수정하는건 매우 안좋습니다.
원본 배열을 다루지 않고 새 배열을 반환하는게 좋습니다.
#sortByName(restaurants: Restaurant[]): Restaurant[] {
return [...restaurants].sort((a, b) => a.name.localeCompare(b.name));
}
이런식으로 바꿔볼수 있을거 같아요
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.
원본 배열을 다루지 않도록 노력하는 데 놓치게 되는 것들이 있네요 🥲 localeCompare에 대해서는 처음 알았는데, 좋은 것 같아요!
string.localeCompare(compareString[, locales[, options]])
- compareString: 비교 대상 문자열
- locales (선택적): 비교에 사용할 로케일
- 문자열이 동일하면 0을 반환하고,
비교 대상 문자열이 더 앞에 오려면 음수를 반환하고,
뒤에 오려면 양수를 반환
#sortByDistance(restaurants: Restaurant[]): Restaurant[] { | ||
return restaurants.sort((a, b) => a.distance - b.distance); | ||
} | ||
} |
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/domain/RestaurantManager.ts
Outdated
this.#restaurants.push(restaurant); | ||
} | ||
|
||
filteredAndSortedByOptions(category: Category, option: Option) { |
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.
filteredAndSortedByOptions 의 반환 배열 타입을 명시해보는건 어떨까요?
@@ -0,0 +1,63 @@ | |||
// eslint-disable-next-line |
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.
js에서 ts파일을 import하려고 하면, 다음과 같은 eslint 에러가 발생해서 추가했습니다.
경로 설정에는 문제 없는 것 같은데 어떻게 해결할 수 있을까요?
- Unable to resolve path to module '../domain/RestaurantManager'.
- Missing file extension for "../domain/RestaurantManager"
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.
사실 위처럼 eslint-disable-next-line 를 사용해도 무방하다고 생각합니다~
해결방법은 여러개가 있는데 .eslintrc 파일을 생성해서 수정하셔도 되고,
"settings": {
"import/extensions": [".js", ".jsx", ".ts", ".tsx"],
"import/parsers": {
"@typescript-eslint/parser": [".ts", ".tsx"]
},
"import/resolver": {
"node": {
"extensions": [".js", ".jsx", ".ts", ".tsx"]
}
}
}
import 하실때 가져오려는 파일이름을 명확하게 해도 됩니다.
import RestaurantManager from '../domain/RestaurantManager.ts';
만약 위를 했는데도 안되면 그냥 기존처럼 사용하셔도 됩니다~
if (!this.#checkLinkInputVaildity()) { | ||
this.#linkInput.setCustomValidity('유효하지 않은 링크입니다.') | ||
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.
여기 파일과 크게 관련없을수도 있는데 ux적으로 궁금한게,
폼 제출을 시도할 때마다 유효성 메시지를 초기화하는거 같은데 그러면 사용자가 입력을 수정한 후에도 유효성 메시지가 사라지지 않는 문제가 있을 수가 있습니다. 입력할때도 초기화해주는게 궁금합니다.
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.
현재 다시 확인해보니 폼을 제출할 때 유효성 검사 에러 메시지도 제대로 출력해내고 있지 못한 것 같습니다. 다음사항과 함께 고려해 2단계에서 수정해보도록 하겠습니다.!!
음식점 아이템 추가 후 다시 폼 진입 시 scroll이 최상단으로 가지 않는 문제
validator 추가 1. 같은 이름의 음식점은 추가할 수 없다.
validator 추가 2. 설명은 최대 300자로 제한한다.
this.querySelector('.restaurant__name').innerHTML = name; | ||
this.querySelector('.restaurant__distance').innerHTML = `캠퍼스로부터 ${distance}분 내 `; | ||
this.querySelector('.restaurant__description').innerHTML = description; | ||
this.querySelector('.category-icon').src = this.#getCategoryIconUrl(category); | ||
this.querySelector('.category-icon').alt = category; |
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.
innerHTML 를 주로 쓰셨는데, innerHTML를 조심히 쓰셔야합니다.
innerHTML을 사용하여 HTML을 동적으로 생성하는 것은 편리하지만, 사용자 입력을 그대로 반영할 경우 XSS(Cross Site Scripting) 공격에 취약할 수 있습니다. 따라서 텍스트만 추가하고 싶을땐, innerHTML 대신 textContent, 컨텐츠,노드를 넣을땐 createElement, createTextNode, setAttribute 같은 DOM 메소드를 사용하는 것이 더 안전합니다.
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.
이번 기회에 innerHTML과 textContent를 구분해서 사용해야 함을 알게 됐어요!
innerHTML
: 요소 노드의 콘텐츠 영역 내에 포함된 모든 HTML 마크업을 문자열로 반환textContent
: HTML 태그를 해석하지 않고 순수한 텍스트로만 처리, 외부에서 입력받은 데이터를 콘텐츠로 사용할 때는 textContent를 사용하여 HTML 태그를 해석하지 않고 텍스트로만 처리하는 것이 좋음
src/view/components/Select.js
Outdated
attributeChangedCallback() { | ||
this.#createOptions(); |
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.
nit) nit은 꼭 반영이 아닌 참고 사항입니다~
attributeChangedCallback에서는 속성이 변경될 때마다 옵션을 재생성하는 코드로 보입니다.
여기서 속성이 많다면 성능적으로 이슈가 될수 있습니다.
변경된 속성에 따라 조건적으로 처리를 추가하여 불필요한 작업을 방지하는 것은 어떨까요?
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.
미처 생각하지 못한 부분을 짚어주셔서 감사합니다!
|
||
padding: 8px; | ||
|
||
border: 1px solid var(--grey-200); |
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.
색상을 변수화 시켜두는거 좋아요 👍
} | ||
|
||
.restaurant__description { | ||
display: -webkit-box; |
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.
이번 미션은 UI 디자인이 목적이 아니었기 때문에 미션 저장소에서 templates 하위에 아이콘 png 파일과 index.html, style.css 파일을 제공했고, 이를 활용하여 미션을 진행하라고 했습니다. 그래서 주의깊게 살펴보지 않고 진행하느라 이런 리뷰가 달린 것 같습니다.
조금 아쉬웠던건 class명이나 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.
위의 스타일 클래스만 봐서, 정확한 내용은 좀더 들어봐야겠지만, restaurant__description 와 category-icon 이런것처럼 클래스명이 통일 되지 않는다는 느낌이 들었습니다.
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.
- 하이픈(-) : 여러 단어를 하나의 클래스 이름으로 결합할 때 사용
- 더블 언더스코어(__) : 클래스 이름을 블록(block), 요소(element), 수정자(modifier)로 구분하여 구조화할 때 사용
- 원본 배열이 아닌 복사본으로 sorting - filteredAndSortedByOptions 메서드의 반환타입 명시
- 임포트 파일명 수정 - global.css 에서 분리 - 필요없는 주석 삭제
1️⃣ 질문 : 웹 컴포넌트에 대해이번 미션의 학습 목표와 학습자료의 내용은 다음과 같았습니다.
저를 포함한 대부분의 크루들은 템플릿 리터럴보다는 웹 컴포넌트가 재사용 측면에서 용이하기 때문에 이를 선택하여 이번 미션을 진행했습니다. 그런데 리뷰어 분들의 의견을 살펴보면 웹 컴포넌트를 써야만 하는 이유가 무엇인지 되물으며 부정적인(?) 의견을 내보이는 것 같더라구요. 케빈의 의견은 어떤지 궁금해요~! 2️⃣ 질문 : 프로젝트 구조에 대해 (feat: 페어 렛서가 받은 피드백)
페어는 이렇게 피드백을 받고 코드 구조를 수정하고 있더라구요. 리뷰어마다 성향과 피드백 방향성이 다르기 때문에 페어의 피드백을 구경하는 것도 하나의 재미처럼 느껴지네요. 그런데 미션이 컴포넌트로 관리하길 원했기 때문에 view 디렉토리 대신 components 디렉토리로 관리하도록 수정해야 할까요? 당장 수정할 것은 아니지만,,ㅎ 케빈의 생각은 어떠한지 궁금해져 질문드립니다~! |
결론부터 말하면 저도 이 미션에서 웹 컴포넌트를 쓰는것에 부정적인 의견입니다! |
view/components 내에서 단순히 ui를 표시하는 컴포넌트와 다른 레이어와 교류하는 컴포넌트를 구분하셔서 controller에 있는 로직들이 컴포넌트 단으로 가야하지 않나 생각이 드는데, (혹은 WebController 자체를 Component로 바라보거나) 이 부분은 일정이 많이 벗어나지 않는 선에서 수정 부탁드려요! 특히 구조를 살펴보면, 내가 만드는 서비스를 정확하게 파악하고 유연하게 구조를 잡기보단, 이미 구조를 나는 웹 컴포넌트를 쓸꺼야 하고 정한뒤 요구사항을 구현하기 위해 View 단이나 비지니스를 맞춘 거 같아 현재 구조에서는 지금 변경하면 2단계 미션에 �영향이 갈수도 있다고 생각해 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.
안녕하세요 초코!
수정해주신 커밋 모두 확인했습니다 1단계 미션 하느라 고생많으셨습니다 👍
남겨주신 질문에 대해 추가 답변 달았으니 한번 읽어보시는걸 추천드려요~
1단계 미션은 여기서 머지할게요! 2단계 미션에서 만나요~
안녕하세요 케빈, 🍫초코🍫입니다.
이번 미션의 요구사항은 타입스크립트를 사용하는 것이었는데요. 아직 자바스크립트도 익숙치 않은 상황에서 타입스크립트를 마주하는 것은 쉽지 않았습니다. 미션을 통해 타입스크립트를 처음 접한 것이라 페어의 주도 하에 1단계를 진행했는데요, 2단계는 혼자서 해낼 수 있도록 학습해보겠습니다~
🏃♀️ 실행 방법
🔗 배포 링크
실행 결과
📍 학습 목표
✅ 프로그래밍 요구 사항
📂 폴더 구조
파일 설명
📜 파일 트리 보기
개선하고 싶은 부분