Skip to content
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 #350

Merged
merged 17 commits into from
Jan 13, 2024
Merged

[안소연] week12 #350

merged 17 commits into from
Jan 13, 2024

Conversation

sozign
Copy link

@sozign sozign commented Jan 7, 2024

주요 변경사항

  • next, ts로의 마이그레이션을 진행했습니다

멘토에게

  • 총체적으로 미완성 상태이지만 빨리 리뷰를 받고 수정하는게 나을 것 같아서,, 제출합니다
  • 로그인/회원가입 페이지는 아직 제작하지 못했습니다
  • /folder 페이지만 모두 타입스크립트로 전환 완료된 상태입니다
  • useEffect 대신에 getStaticProps나 getServerSideProps 등을 활용해보는 것 까지 해보려 했으나 기존에 context 를 활용해서 짠 코드가 대부분이어서 어떻게 바꿀지 고민이 되더라구요,, 관련해서 조언 부탁드립니다
  • 실무에서 typescript를 어떻게까지 사용하는지가 궁금해졌는데 혹시 추천해주실만한 오픈소스나 용례 위주의 컨텐츠가 있을까요?

@sozign sozign requested a review from domuk-k January 7, 2024 14:04
@sozign sozign marked this pull request as ready for review January 7, 2024 14:20
@sozign sozign self-assigned this Jan 8, 2024
@sozign sozign added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성 죄송합니다... labels Jan 8, 2024
Copy link
Collaborator

@domuk-k domuk-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!

총체적으로 미완성 상태이지만 빨리 리뷰를 받고 수정하는게 나을 것 같아서,, 제출합니다

  • 좋은 전략이에요 👍

useEffect 대신에 getStaticProps나 getServerSideProps 등을 활용해보는 것 까지 해보려 했으나 기존에 context 를 활용해서 짠 코드가 대부분이어서 어떻게 바꿀지 고민이 되더라구요,, 관련해서 조언 부탁드립니다

  • context를 활용한 코드를 대체하면 됩니다...!? 각 훅이 필요한 이유를 이해하고, 적절한 사용 예시라고 생각하시면 바로 그렇게 시도해보시면됩니다. 아래 코멘트로 예시를 드렸습니다

실무에서 typescript를 어떻게까지 사용하는지가 궁금해졌는데 혹시 추천해주실만한 오픈소스나 용례 위주의 컨텐츠가 있을까요?

  • 대부분은 추론할 수 있는 경우라서 작성하지 않지만, 빈번하게는 컴포넌트/함수/훅에 파라미터타입에 가장 많이 사용합니다. 별도로 간단한 수준의 오픈소스는 떠오르지않는데, 멘토링때 소개드려볼게요

//프로젝트에서 사용되는 환경을 정의합니다. 여기서는 브라우저, ECMAScript 2020, 그리고 Node.js 환경에서 코드를 실행하는 것으로 설정
"browser": true,
"es2020": true,
"node": true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 node에서도 실행가능한가보군요? 사실 Nextjs에서는 컴포넌트 별로, 또는 특정 위치의 함수의 경우에 server/ client 한 곳에서만 동작하게 의도되기도해서, 이렇게 eslint설정으로 드러낼 필요는 없어 보여요. 혹시 특정 코드에서, 특정 런타임 환경이 제공하는 host 객체를 사용해서 이 처리가 필요했던 걸까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전 프로젝트 때 사용했던 세팅 코드를 가져온 건데 미처 뜻을 알지 못하고 사용한 부분들이 있습니다.. 😅 이참에 공부해봐야겠네욥

.vscode/settings.json Show resolved Hide resolved
@@ -1,5 +1,5 @@
{
"name": "fe-weekly-mission",
"name": "part3-weekly-mission-next-with-ts",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json의 이름은 이 프로젝트 하나를 하나의 패키지로 보고 지칭하는 표현입니다. 패키지를 배포해보시는 경혐을 하거나, npm workspace 기능을 사용하신다면 더 잘 느끼실 수 있을 거 에요.

사실 이 레포 또는 넥스트앱을 누가 참조하진 않아서 상관없지만 지금 바꾸신 이름도 좋네요.

src/components/common/AddLinkBar.tsx Show resolved Hide resolved
src/components/common/CardForShared.tsx Outdated Show resolved Hide resolved
src/hooks/useClickOutside.js Outdated Show resolved Hide resolved
src/hooks/useModal.tsx Show resolved Hide resolved
const result = await fetch(API.SAMPLE_FOLDER);
const { folder } = await result.json();
const { links } = folder;
folder.links = convertToSnakeCase(links);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다 다만, js/ts에서 일반적으로 캐멀케이스가 글로벌한 컨벤션이니만큼 camelCase로 통일하는게 좋아보입니다

src/hooks/useToggle.js Outdated Show resolved Hide resolved
Comment on lines +40 to +51
const contextValue: SearchContextValue = {
searchValue,
setSearchValue,
selectedFolder,
setSelectedFolder,
folderList,
setFolderList,
linkList,
setLinkList,
};

return <SearchContext.Provider value={contextValue}>{children}</SearchContext.Provider>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하나의 맥락(Context)에 담는 것은 성능에도, 일반적으로 의도전달에도 안좋아서, 고민해보시면 도움이 될거같습니다

@sozign
Copy link
Author

sozign commented Jan 9, 2024

멘토님 정성스러운 리뷰 감사합니다! 남겨주신 리뷰사항 참고해서 이번주차 pr 올리도록 할게요 💪

@domuk-k domuk-k merged commit 2f158a5 into codeit-bootcamp-frontend:part3-안소연 Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성 죄송합니다...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants