[송시은] Sprint5 #181
[송시은] Sprint5 #181
Conversation
[Fix] delete merged branch github action
…-송시은-sprint1 [송시은] Sprint1
…asswordToggleIcon, SignUp components
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
시은님 5번째 미션 제출 고생하셨습니다!
기존에 작업하시던 것들도 옮기시느라 더 고생하셨을 것 같아요.
PR도 잘 정리해주셔서 리뷰할 때 도움이 되었습니다.
다음 미션도 화이팅이에요~
-
mission_template_codes 라는 파일에 그전주차 코드들이 모여있는 것 같습니다. 이런 코드가 github에 올라와야 할 이유가 없을 것 같아요. 또한 git같은 버전관리를 사용하는 이유가 특정 시점으로 돌아가기 위해서이니 이렇게 백업파일/폴더를 추가하실 필요가 없을 것 같습니다.
-
scss를 사용하면서 스타일을 아래의 예시처럼 어떤부분은 개별 클래스로 주었고 어떤부분은 클래스를 중첩해서 주었는데 이렇게 섞어쓰는게 일반적인지, 아니면 하나의 방향으로 통일해서 쓰는지 궁금합니다. 하나로 통일한다면 어떤 방식이 좋을까요? ... 같은 맥락에서 아래와 같은 경우 selector를 계층적으로 스타일을 중첩해서 적용하는게 좋을지 아니면 span, p 에 각각 className을 주고 스타일을 적용하는게 좋을지 판단기준이 궁금합니다. (중복되는 경우는 className을 적용하는데 중복되지 않는 경우는 어떤게 나은지, 이부분은 사이트 전체적으로 통일하지 않고 섞어서 쓰는 편인지도 궁금합니다.) 등:
가독성과 일관성을 위해 룰을 정하시는 것처럼 하나로 통일해서 작성하는 것이 좋습니다만 아니라고 해도 룰로 정해진 것이 없다면 크게 상관없다고 생각합니다. 저라면 scss를 쓰시니 nesting 방식으로 사용할 것 같습니다. 다만 이는 이것이 더 좋다는 의미는 아닙니다. -
스타일을 적용하면서 고민했던 부분이 컴포넌트화하는게 나을지 아니면 className을 두개주고 하나는 공통 스타일이 적용되는 클래스네임, 다른 하나는 개별적으로 적용되는 스타일, 이렇게 하는게 나을지 였습니다. 페이지에 따라 공통적인부분은 컴포넌트로 빼는게 나을거 같은데 하나의 페이지에서 자주 사용되는 것은 두개의 클래스네임을 주는편이 나은건지, 이 부분도 어떤 기준에서 판단하면 좋을지 궁금합니다. 그리고 예를 들어 기본스타일은 같은데 위치랑 크기만 달라지는 버튼의 경우 컴포넌트화하면 달라지는 부분은 prop으로 받아서 아래와 같이 인라인으로 스타일 적용하게 되는데, 인라인 스타일 적용을 지양하라고 배웠는데 이렇게 해도 되는지, className 두개 쓰는 편이 더 나은건지 궁금합니다. 또 만약 인라인으로 쓴다면, 아래 예시에서 positon의 경우 항상 absolute일때 아래처럼 인라인에 표시하는게 좋은지 아니면 항상 같은 스타일들은 scss파일을 따로 만들어서 거기에 써야할까요?:
우선적으로 인라인 스타일링을 해야만하는 이유가 없다면 하지 않으시는 것이 좋습니다. 지금 예시코드에서도 다른 방법이 있기 때문에 위에처럼 size, top, right값을 받아서 처리해야하는지 의문입니다. 저라면 className을 props 로 받기 때문에 관련 속성을 전부 없앨것 같습니다. 이러한 판단은 상황에 따라 다르기때문에 안티패턴을 피하려고 하고 좋은 것을 따르려고 하되 가장 중요한 것은 요구사항을 구현할 수 있는지 여부입니다. 이러한 기준을 세우시고 그때 그때 판단하시면 됩니다~ -
base.css에 적용한 버튼 공통 스타일이 있었는데 때로는 이 스타일과 전혀 다른 버튼을 만들어야할때 all: unset을 하는게 좋을까요 아니면 해당 스타일들을 예를들어 background-color: blue; 로 base에 되어있을떄 새로운 스타일 버튼에서 background-color: transparent; 이런식으로 하나씩 바꾸는게 좋을까요?:
이런 경우는 base.css에서 버튼 공통 스타일을 관리하지 않으시는 편이 좋아보입니다. 전혀 다른 버튼을 만들어야 한다면 base에 있는 버튼 스타일이 공용이 아니라는 의미입니다. -
h1-h6같은 경우는 스타일을 base에 선언하지 않고 쓰는편인가요? 페이지별로 시멘틱을 고려하면 h2인데 스타일은 다른페이지의 h2와 달라서 처음엔 h1-h6기본 스타일을 base에 선언했다가 그냥 다 지우고 개별 페이지에서 각각 적용했는데 이렇게 하는게 맞는걸까요?:
이는 프로젝트마다 다를 것 같습니다. 태그로 기본 스타일을 주기보다 컴포넌트화해서 사용하는 경우가 많을 것 같습니다. -
css reset, base.css, vairable.css는 전역에서 쓰이고 scss문법을 안써서 .css로 했는데 앱 전체적으로 scss+css module을 사용하는데 이렇게 해도 되는지, 보통 어떻게 하는지 궁금합니다.: 혼용해야 하는 이유가 있다면 그렇게 하셔도 됩니다. 다만 "보통" 어떻게 하는지는 제가 정확한 통계가 있는 것이 아니라 답변드리기가 어렵습니다~ -
아래 두 가지는 피그마 요구사항에서 변경한 부분인데 이렇게 해도 괜찮을까요?: 디자인은 가이드 라인으로 무조건 따라야하는 것은 아니며 이를 변경하실 수 있기 때문에 자세한 수치들은 변경하셔도 됩니다!
| import styles from './Header.module.scss'; | ||
|
|
||
| const Header = () => { | ||
| const isItemsPage = window.location.pathname === ROUTES.ITEMS; |
There was a problem hiding this comment.
💊 제안
디자인을 구현하시려고 이렇게 하신걸까요? 추후 로그인시 프로필 이미지를 보여주고 로그아웃 시 로그인 버튼을 보여주는 코드로 바꾸시면 좋겠습니다~
There was a problem hiding this comment.
이 부분은 아이템 페이지일 경우 네비게이션 중고 마켓이 active되서 파란글씨가 되야해서 사용했습니다.
There was a problem hiding this comment.
❗️ 수정요청
어떤 form 과 관련된 hook인지 이름을 더 자세히 명명해주시면 더 좋을 것 같아요.
코드상 auth에서만 사용될 것 같은데 이런 경우 src 아래의 hooks 폴더보다 사용처에 hooks 폴더를 생성한 후 위치시키시는 것을 추천드려요.
There was a problem hiding this comment.
💬 여담
React Router를 사용하고 계신데 향후 라우트가 많아지면 Route들을 별도의 파일로 분리하면 App 컴포넌트가 더 깔끔해지고, 라우트 관리도 쉬워집니다.
| <button | ||
| key={page} | ||
| onClick={() => onPageChange(page)} |
There was a problem hiding this comment.
💊 제안
버튼 태그로 잘 작업하셨어요! 이런 경우 아래처럼 이벤트 객체를 통해 접근하실 수도 있습니다.
개인적으로 코드도 깔끔해지고 이벤트 객체로 받는것이 더 적절하다고 생각해서 추천드려요!
| <button | |
| key={page} | |
| onClick={() => onPageChange(page)} | |
| <button | |
| key={page} | |
| value={page} | |
| onClick={onPageChange} // onPageChange 함수에 event 자체를 인자로 받아 event.target.value 이렇게 접근 |
| import defaultProductImg from '@/assets/images/default_product.svg'; | ||
| import styles from './ProductCard.module.scss'; | ||
|
|
||
| const ProductCard = ({ product }) => { |
There was a problem hiding this comment.
💊 제안
product 라는 객체로 props를 받기보다 images, name... 이렇게 받으시는게 더 좋아보여요.
지금 해당 컴포넌트에서 다른 props를 받는것이 아니라서 객체로 받으실 필요가 없어보입니다~
// 추천하는 방식
const ExComponent = ({ images, name }) => { ... }
// 아래의 경우 다른 props를 받기 때문에 객체로 묶어 받음
const ExComponent = ({obj, classname}) => { ... }| const res = await fetch( | ||
| `${baseUrl}/products?page=1&pageSize=1000&orderBy=favorite`, //서버에 좋아요 순으로 정렬한 상위 N개의 데이터를 보내주는 전용 endpoint가 없어서 일단 임시로 이렇게 처리 | ||
| ); |
There was a problem hiding this comment.
❗️ 수정요청
orderBy param으로 정렬기준을 넘겨주고 pageSize로 원하는 상품 개수를 보내주면 됩니다~ 지금의 경우 아래처럼 작성하시면 될거에요.
| const res = await fetch( | |
| `${baseUrl}/products?page=1&pageSize=1000&orderBy=favorite`, //서버에 좋아요 순으로 정렬한 상위 N개의 데이터를 보내주는 전용 endpoint가 없어서 일단 임시로 이렇게 처리 | |
| ); | |
| const res = await fetch(`${baseUrl}/products?page=1&pageSize=4&orderBy=favorite`); |
| const filteredProducts = products.filter((product) => | ||
| product.name.toLowerCase().includes(searchQuery.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
❗️ 수정요청
프론트에서 검색 인풋의 값에 따라 상품요소를 필터링하도록 작성하셨는데, 이렇게 되면 전체 상품 목록에서 필터링 되는 것이 아니라, 불러온 데이터 내에서 필터링하는 것이라 의도한 바와 다르게 동작할거에요!
이런 경우 Products api의 Param을 통해 구현하시면 됩니다.
검색 키워드는 search 값으로 넘겨서 서버에서 응답온 값을 이용해주세요.
|
|
||
| return ( | ||
| <div className={layoutStyles.layoutWrapper}> | ||
| {!isAuthPage && <Header />} |
There was a problem hiding this comment.
💊 제안
이렇게 처리도 가능하지만 사용하시는 라우팅 라이브러리의 기능을 최대한 활용해서 구현하시는 것을 추천드려요!
아래 문서를 보시고 각 route에 따라 layout를 구성해보시는 것을 추천드립니다.
| <li | ||
| key={opt.value} | ||
| onClick={() => { | ||
| onChange(opt.value); | ||
| setIsOpen(false); | ||
| }} | ||
| className={styles.option} | ||
| > | ||
| {opt.label} | ||
| </li> |
There was a problem hiding this comment.
❗️ 수정요청
이런 클릭 가능한 요소의 경우 버튼을 사용해주세요. 더 적절한 태그가 있는데 이미지 태그에 onClick 을 주게되면 button에게 기대하는 동작을 제대로 수행하지 못합니다!
| <li | |
| key={opt.value} | |
| onClick={() => { | |
| onChange(opt.value); | |
| setIsOpen(false); | |
| }} | |
| className={styles.option} | |
| > | |
| {opt.label} | |
| </li> | |
| <li key={opt.value} className={styles.option}> | |
| <button | |
| type="button" | |
| value={opt.value} | |
| onClick={() => { | |
| onChange(opt.value); | |
| setIsOpen(false); | |
| }}> | |
| {opt.label} | |
| </button> | |
| </li> |
4 participants
요구사항
배포 링크: codeit-sprint15-mission.netlify.app
기본
중고마켓
중고마켓 반응형
심화
주요 변경사항
이전 미션 수정했던 부분(지난 미션4 JS부분)
이번 미션 요구사항은 아니지만 추가한 부분
스크린샷
멘토에게
그리고 예를 들어 기본스타일은 같은데 위치랑 크기만 달라지는 버튼의 경우 컴포넌트화하면 달라지는 부분은 prop으로 받아서 아래와 같이 인라인으로 스타일 적용하게 되는데, 인라인 스타일 적용을 지양하라고 배웠는데 이렇게 해도 되는지, className 두개 쓰는 편이 더 나은건지 궁금합니다.
또 만약 인라인으로 쓴다면, 아래 예시에서 positon의 경우 항상 absolute일때 아래처럼 인라인에 표시하는게 좋은지 아니면 항상 같은 스타일들은 scss파일을 따로 만들어서 거기에 써야할까요?
all: unset을 하는게 좋을까요 아니면 해당 스타일들을 예를들어background-color: blue;로 base에 되어있을떄 새로운 스타일 버튼에서background-color: transparent;이런식으로 하나씩 바꾸는게 좋을까요?