-
Notifications
You must be signed in to change notification settings - Fork 57
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
[허우림] week12 #365
The head ref may contain hidden characters: "part3-\uD5C8\uC6B0\uB9BC-week12"
[허우림] week12 #365
Conversation
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-config-next": "13.5.6" | ||
"@typescript-eslint/eslint-plugin": "^6.18.1", | ||
"@typescript-eslint/parser": "^6.18.1", | ||
"axios": "^1.6.5", |
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.
dependencies와 devDependencies를 잘 분리해주셨네요!
axios도 dependencies에 위치하면 좋을 것 같습니다.
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.
axios
를 설치할 때 분명히 npm install axios
로 설치했는데, devDependencies
에 가있었군요. 앞으로는 라이브러리 설치 시에 package.json
더 꼼꼼히 확인해서 구분 잘 하도록 하겠습니다. 감사합니다~
@@ -157,7 +160,7 @@ export default function Folder() { | |||
<div className={styles.folderAddContainer}> | |||
<div className={styles.folderWrapper}> | |||
{folderLists ? ( | |||
folderLists.map((folder: any, i) => { | |||
folderLists.map((folder: any, i: number) => { |
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.
list
라는 이름은 그 자체로 복수의 데이터임을 나타냅니다.
folderList
혹은 folders
같은 이름이 더 적절하겠네요!
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.
네 앞으로 lists
, datas
라는 변수명은 지양하고 변수 네이밍에 더 신경쓰도록 하겠습니다! 감사합니다.
email: '', | ||
image_source: '', | ||
}); | ||
const [folderDatas, setFolderDatas] = useState<FolderProps>({ |
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.
data
는 그 자체로 복수형입니다. folderData
와 같은 이름이 조금 더 적절하겠습니다.
profileImageSource: string; | ||
} | ||
|
||
interface FolderProps { |
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.
Props
는 주로 컴포넌트의 prop을 정의할 때 사용하는 접미사입니다. FolderProps
는 useState가 관리하는 폴더 데이터를 나타내므로 단순히 Folder
로 쓰면 충분하겠네요. 혹은 아래와 같이 인라인으로 타입을 선언해줘도 괜찮습니다.
useState<{
id: string;
name: string;
owner: object;
links?: {}[];
}>();
id: id, | ||
name: name, | ||
email: email, | ||
image_source: profileImageSource, |
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.
특별한 이유가 있는게 아니라면 네이밍 컨벤션도 통일되면 좋겠네요!
imageSource: profileImageSource,
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.
이건 api에서 리턴해주는 데이터 프로퍼티명이 달라서 일부러 image_source로 바꿔주었습니다!
const { setLinkInfo } = useContext(CardContext); | ||
const { id, createdAt, url, title, description, imageSource } = link; | ||
const [mins, setMins] = useState(''); | ||
const [createdDates, setCreatedDates] = useState({ |
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.
year, month, day는 createdAt으로부터 파생된 값이네요.
그렇다면 별도의 state 없이 아래와 같이 단순히 함수 호출로 처리할 수 있습니다.
const [year, month, day] = calCreatedDates(createdAt);
--card-border-rgb: 200, 200, 200; | ||
} | ||
} | ||
@import url('https://cdn.jsdelivr.net/gh/orioncactus/pretendard/dist/web/variable/pretendardvariable.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.
url이 잘못 입력되어 있어서 css를 정상적으로 불러오지 못하고 있었네요.
아래와 같이 수정하면 폰트가 잘 적용됩니다.
@import url('https://cdn.jsdelivr.net/gh/orioncactus/pretendard/dist/web/variable/pretendardvariable.css'); | |
@import url('https://cdn.jsdelivr.net/gh/orioncactus/pretendard/dist/web/static/pretendard.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.
오 감사합니다!!
const { clickedKebabOption, setClickedKebabOption, closeKebab } = | ||
useContext(KebabContext); | ||
const { folderLists } = useContext(FolderContext); | ||
const { linkInfo } = useContext(CardContext); |
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
컴포넌트에서 링크의 url을 사용하기 위해 CardContext를 사용해주고 계신데요,
링크 url 하나만을 위해 별도의 컨텍스트를 생성하는게 적절한지 잘 모르겠습니다.
공유가 필요한 데이터의 양이 그리 많은 편이 아니고, 컴포넌트 중첩 단계도 깊지 않으니 명시적으로 prop을 전달해서 데이터의 흐름을 이해하기 쉽도록 컴포넌트를 구성해보는건 어떨까요?
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.
두 번이상 하위 컴포넌트로 props를 전달하면 프롭스 드릴링이라고 알고 있어서, 혹시나 컴포넌트의 구성이 바뀌거나 했을 때를 고려하여 최대한 프롭스 드릴링을 피하는 게 좋다고 생각하여 context를 사용했던 것이었습니다! 근데 이렇게 context에 저장할 데이터양이 적고 뎁스가 깊지않은 경우에는 context를 사용하지 않는 게 괜찮을 수도 있군요. 코드에 잘 반영하였습니다. 감사합니다~
description, | ||
createdDates, | ||
}: Props) { | ||
const { setIsKebabClicked, isKebabClicked } = useContext(KebabContext); |
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.
각 케밥 버튼의 상태는 CardInfo 컴포넌트 내부에서만 관리되고 있네요.
전역적으로 공유가 필요한 성격의 데이터도 아니고, 컴포넌트 내부 상태로 관리하는 편이 데이터 흐름을 더 쉽게 파악할 수 있을 것 같습니다.
예시:
const [isKebabClicked, setIsKebabClicked] = useContext(false);
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.
그러네요! 한 컴포넌트 안에서만 쓰는 state
인데 context
를 사용할 필요가 없었네요!
그런데 멘토님 예시코드에서 useContext
가 아니라 useState
사용하라고 말씀하시는 거 맞을까요?
|
||
export default function App({ Component, pageProps }: AppProps) { | ||
return <Component {...pageProps} /> | ||
return ( | ||
<FolderContextProvider> |
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때 같이 올리도록 하겠습니다😃 |
요구사항
기본
주요 변경사항
스크린샷
멘토에게
이슈
pages/index.tsx
를 제외한 다른 페이지에서 Pretendard 폰트가 적용되지 않습니다.index.tsx
의<Head>
에서 pretendard 폰트를 임포트하고,globals.css
에서* { font-family: 'Pretendard';}
를 주고,pages/_app.tsx
에서import '@/styles/globals.css';
를 하는 방식으로 사용하였습니다. 아직 이유를 찾지 못하였습니다.질문
context
를 제가 올바르게 사용한 것인지 잘 모르겠습니다. 더 개선할 수 있는 방법이 있다면 무엇인지 궁금합니다.현재 사용하는 context들과 그 목적
CardContext
: 각 카드에 있는 링크의 info를 저장하여, 모달에 보여주기 위해FolderContext
: 각종 클릭된 옵션, 추가된 링크, 폴더 목록 등을 저장하고 하위 컴포넌트에서 사용하기 위해KebabContext
: 각 카드에 있는 케밥 버튼이 클릭되었는지, 케밥버튼 하위의 옵션 중 어떤 옵션이 선택되었는지, 케밥버튼을 눌렀을 때 나오는 모달을 닫기 위해