-
Notifications
You must be signed in to change notification settings - Fork 0
feat: ScenePage 제작 및 Bass Scene 삽입 #18
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
Conversation
@@ -13,6 +13,7 @@ const ResponsiveFlexGrid = <T extends object>({ | |||
return ( | |||
<FlexWrapper> | |||
{mappingData.map((data, idx) => ( | |||
//biome-ignore lint/suspicious/noArrayIndexKey: Using index as key is acceptable here since the data is static and does not change. |
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.
혹시 주석으로 바이옴 오류를 제거한 이유가 있으실까요?! 하나의 파일에서만 린트 에러를 끄고 싶을 때는 이렇게 주석을 이용하기도 한다고 해서, 의도하신 부분인지 궁금합니다!!
조사해본 결과, 아래처럼 바이옴 설정 파일에서 린트 오류를 직접 끌 수도 있다고 합니다. 둘 중 더 마음에 드는 방식을 선택하셔도 좋을 것 같아, 공유드립니다!
"suspicious": {
"noArrayIndexKey": "off"
}
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.
주석으로 린트 에러를 제거한 이유는, 기본적으로는 제한하되 한정된 상황에서만 예외적으로 허용하기 위함입니다!
map 함수에서 index를 key로 사용하는 경우는 기본적으로 지양하게끔 설정해두는 것이 코드의 안정성 측면에서 좋을 것이라 생각합니다.
다만, 이번 컴포넌트의 경우 mappingData에 key로 사용할 수 있는 식별자가 없는 상황도 발생할 수 있기에 예외적으로 허용했습니다.
(해당 데이터에 id 등 고유 식별자를 필수로 갖게끔 타입을 설정하고, 해당 필드를 map 메서드의 key로 사용하는 것이 더 적절한 방향이라고 생각합니다. 디테일한 부분이기에 우선 방학 내에 프로젝트를 마무리하고 리팩토링 해보아도 좋을 것 같습니다)
forEach 대신 for-of 사용을 강제하는 등의 스타일 관련 린트 경고는 개인 취향이나 팀 스타일에 가까운 부분이기 때문에, 제나가 제안해준 것처럼 biome.json 설정을 통해 명시적으로 허용 여부를 조정하는 편이 더 깔끔하고 일관성 있는 해결책이 될 수 있을 것 같습니다!
저도 pages 단으로 옮기는 것에 찬성합니다! |
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.
SceneCard는 "어떻게 보여줄지"만 책임지고 어떤 데이터를 받을지는 페이지가 결정한다는 측면에서
SceneCard 내부에 위치한 consts를 pages 단으로 옮기고 싶은데, 다들 어떻게 생각하시는지 궁금합니다!
저도 현재 구조에서는 pages 단으로 옮겨도 좋을 것 같습니다!👍
파일도 깔끔하게 정리해주셨군요!👍👍
블루 넘 고생하셨습니다! ㅎㅅㅎ
씬 연결 방법을 찾아서 노션에 정리해주시고 이렇게 적용해주신 덕분에 블루 코드보면서 씬 적용 수월하게 할 수 있겠네요!✨
감사합니다 ㅎㅎ 수고하셨어요!🙇🏻♀️
package.json
Outdated
@@ -20,7 +20,7 @@ | |||
"three": "^0.177.0" | |||
}, | |||
"devDependencies": { | |||
"@biomejs/biome": "^1.9.4", | |||
"@biomejs/biome": "2.0.5", | |||
"@eslint/js": "^9.25.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.
현재는 biome을 사용하고 있으니 eslint 는 삭제해도 괜찮지 않을까요?😃
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️⃣
ScenePage
컴포넌트 제작2️⃣
BassScene
씬 삽입ScenePage
컴포넌트 기반, children props으로 문구 추가 (Click the bass!)SceneCard
의 a 태그를 Link 로 수정3️⃣ 스타일 파일 형식 ts로 통일
💬리뷰 요구사항
✅ SceneCard 내부의 consts 위치
SceneCard는 "어떻게 보여줄지"만 책임지고 어떤 데이터를 받을지는 페이지가 결정한다는 측면에서
SceneCard 내부에 위치한 consts를 pages 단으로 옮기고 싶은데, 다들 어떻게 생각하시는지 궁금합니다!
(이번에 해당 consts 파일을 수정하면서, 현재 구조보다 pages 단에서 관리하는 편이 더 적절할 수 있겠다는 생각이 들었습니다.)
(+ 추가로, 빌드한 파일을 불러와 씬을 보여주는 방식 외에 좀 더 효율적인 방식에 대한 의견이 있다면 언제든 환영합니다 ✨)