-
Notifications
You must be signed in to change notification settings - Fork 21
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
[정성현] sprint7 #104
The head ref may contain hidden characters: "React-\uC815\uC131\uD604-sprint7"
[정성현] sprint7 #104
Conversation
- api 함수 공통 코드 분할 - 드롭다운 옵션을 받아 재사용 가능하도록 개선 - 페이지네이션바 기능 수정 - 미디어 변경 시 첫 페이지로 초기화 - 화살표 클릭 시 5페이지씩 이동 - 공통 컴포넌트에 넘기는 프로퍼티 최소화 - 레이아웃에서 헤더 컴포넌트 분리 - reset.css 속성 선택자 제거(CSS 우선순위 문제) - 오타 수정
- additem 경로로 접근 가능한 상품 등록 페이지 라우팅
- 상품 등록 페이지를 이루는 html 요소 작성 - 기본적인 main 요소 CSS 작성
- 기초적인 CSS 적용
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.
성현님 이번 PR 제출하시느라 고생하셨습니다!
스프린트 미션의 요구사항은 여러분의 학습을 돕기위한 가이드 라인일뿐
꼭 하셔야 하는 의무는 아니니 성현님 진도 맞춰서 하시면 충분합니다~
HTML 내 CSS가 전역->컴포넌트가 아닌 컴포넌트->전역 순서라, 컴포넌트CSS가 전역CSS에 덮이고 맙니다. 리렌더링 or React Router 이슈로 추측되나, 구글링으로 만족스러운 해결책을 찾지 못했습니다. 바닐라 CSS 구조에서 해결 가능할까요?
: 약간 이해가 안되는 것이 있습니다. 전역 스타일과 컴포넌트 개별 스타일이 작성된 css 파일이 있는데 이중 전역 스타일이 개별 스타일을 덮어쓰우신다는 건데, 전역 스타일은 대부분 태그 선택자로 입력하기 때문에 클래스 선택자로 입력하는 개별 스타일보다 우선순위가 낮을텐데 그 순서가 왜 중요한지 이해가 잘 안됩니다. 나중에 더 자세히 설명해주시면 좋을 것 같아요.form fieldset 내에 legend 요소를 사용해보려다가, 외부 flex나 자체 display가 잘 먹히지 않아 사용하지 않았습니다. display:contents를 쓰면 그때부턴 CSS 구조가 동작하긴 하는데, legend처럼 CSS를 무시하고 레이아웃?을 고정하도록 설정된 이유가 무엇지 궁금합니다!
: 이것도 어떤 의미인지 잘 모르겠네요 😅 어떤 외부 display 속성이 어디에 잘 먹히지 않는다는 것인지, 어디에 display cotents를 주셨다는건지, 그래서 어떤 css 구조가 동작했다는 건지 알수가 없습니다. 이것도 나중에 추가 설명해주시면 더 좋을 것 같습니다.
</nav> | ||
<img className="header__profile" src={profileImage} alt="프로필 사진" /> | ||
</header> | ||
<Header /> |
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.
P3:
👍
@@ -15,6 +15,12 @@ export default function ItemListHead({ setParamObj }) { | |||
}, | |||
[setParamObj] | |||
); | |||
const handleDropdownSelect = useCallback( |
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.
P3:
useCallback 을 이용해서 함수를 캐싱할 필요가 있는지 잘 모르겠습니다.
리액트 공식 문서에서도 특별한 이유가 없다면 함수를 꼭 useCallback으로 감쌀 필요는 없습니다.
와 같이 언급하고 있으니
어떤 이유로 useCallback
을 사용하셔야 하는지 한번 고민해보세요.
추천드리는 방법은 우선 작업하실때는 useCallback 같은 캐시함수를 사용하지 않으시고
나중에 필요하실때 추가하시는 것입니다~
https://ko.react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere
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.
해당 함수에 useCallback을 사용한 이유는 무한 리렌더링때문입니다!
- 해당 함수를 Dropdown 의 onSelect 속성으로 넘김
- onSelect 속성이 useEffect 내에서 사용돼 의존성 배열에 담음(의존성 배열에 안 담으면 터미널에서 경고 뜸)
- handleDropdownSelect가 useCallback 안쓰면 무한 리렌더링 발생
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.
하단 Dropdown.js쪽 피드백을 확인해보니, 애초에 useEffect를 안 써도 됐던 것 같습니다😅
말씀해 주신대로 useCallback을 없애겠습니다! 👍
return ( | ||
<section className="addItemForm"> | ||
<form className="form"> | ||
<fieldset className="form__head"> |
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.
P2:
fieldset 태그는 여러 인풋들을 묶을 때 쓰는 태그라서 여기서는 다른 태그를 쓰시는게 좋을 것 같습니다.
밑에서는 아주 잘 사용하셨어요~
onSelect(value); | ||
}, [onSelect, value]); |
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.
P1:
Dropdown 컴포넌트 내부 state인 value가 바뀌었을 때 이를 onSelect 함수로 전달하는 것보다
handleOptionClick 함수에서 onSelect(value)로 전달하는게 더 좋을 것 같아요.
onClick={handleSelectClick} | ||
onBlur={handleSelectBlur} | ||
> | ||
{value === "recent" ? "최신순" : "좋아요순"} | ||
{children.find((child) => value === child.props.value).props.children} |
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.
P2:
value를 바로 props로 받으시면 더 좋을 것 같아요.
자꾸만 자꾸만 진도가 뒤쳐집니다😭
최대한 6주차, 7주차 진도 따라가보겠습니다.
배포 사이트 바로가기
요구사항
기본
상품 등록
심화
상품 등록
주요 변경사항
스크린샷
멘토에게