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

[김법균] Week14 #458

Merged

Conversation

bk-git-hub
Copy link
Collaborator

요구사항

기본

  • 로그인, 회원가입 페이지를 만들어 주세요.
  • 로그인 페이지의 url path는 ‘/signin’, 회원가입 페이지의 url path는 ‘/signup’ 입니다.
  • [ ]

심화

  • 로그인, 회원가입 기능에 react-hook-form을 활용해 주세요.

주요 변경사항

  • api관련 기능들을 추가했습니다

멘토에게

  • 시간을 좀 더 내서 했어야했는데 못했네요..
  • 지난번에 주신 피드백 내용은 반영하지 못했습니다. 이번주 피드백 내용까지 잘 반영해보도록 하겠습니다.
  • 지금 코드 구조가 좀 엉망입니다.

@bk-git-hub bk-git-hub requested a review from 1005hoon May 20, 2024 03:59
@bk-git-hub bk-git-hub added 미완성🫠 죄송합니다.. 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. labels May 20, 2024
Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

@bk-git-hub

법균님! 쉽지 않으셨을텐데 개발하시느라 고생 많으셨어요
아직 이전 피드백이 많이 반영되지 않은 부분도 있고, 반복적으로 이야기하는 요소들이 많아 지금 상황에서는 크게 리뷰를 드릴 내용이 없기도 합니다.
우리 멘토링때 이야기하던 관심사 분리 차원에서 프로젝트를 한번 리팩토링 해 보시면 좋을 것 같아요.
그 외적으로 보이는 요소들에 대해서는 코드단에 리뷰 남겨두었으니 체크 부탁드려요.
고생하셨습니다!

.eslintrc.json Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

이걸로 설정해보는건 어떤가요? 공식문서에서도 recommend 하고있는 strict 형태여서 제안드립니다
https://nextjs.org/docs/app/building-your-application/configuring/eslint

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

역시 말씀드리지만, javascript project convention에 따라 파일명을 제작하는게 좋겠어요
https://google.github.io/styleguide/jsguide.html#file-name

파일 이름의 경우, lowercase와 underscore의 조합으로만 작성을 하고, 리액트 프로젝트 컨벤션에 따라 콤포넌트만 PascalCase 또는 snake-case로 작성을 하는건 어떨까요?

userEmail: string;
}

const Account = ({ profileImgSource, userEmail }: AccountProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

account라기 보단 user-profile이라는 콤포넌트가 더 적합한 이름으로 보이는데 어떻게 생각하시나요?

Comment on lines +4 to +5
profileImgSource?: string;
userEmail: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

prop으로 전달받는 요소의 경우, 우리 프로젝트에서 전반적으로 사용되는 유저 데이터가 존재하나요?
그렇다면 유저 데이터에 대한 타입을 선언해서 그냥 유저 정보를 다 받아오도록 하는게 더 좋지 않을까 생각이 되어요.

type User = {
  profileImgSrc?: string
  email: string
  name: string
  ...
}

interface AccountProps {
  user: User
}

@@ -0,0 +1,30 @@
import styles from './Button.module.css';

interface ButtonProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

button element가 기본적으로 받아올 수 있는 요소들이 존재할텐데요
그걸 받아올 수 있도록 react진영에서 button element에 대한 타입을 사전에 정의해놓았습니다.
Button Props에 위에 설명한 버튼 타입을 상속받아 확장하도록 하는게 좋겠어요

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

모달의 관리 주체와 제어권의 역전을 활용해볼 수 있는 방향을 고민해보면 좋겠어요
https://kentcdodds.com/blog/inversion-of-control

Comment on lines +7 to +9
export const SEARCH_INPUT_ID = 'search-link';
const SEARCH_INPUT_PLACEHOLDER = '링크를 검색하세요';
const SEARCH_INPUT_ICON_ALT = 'Search Icon';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

이 콤포넌트 내부에서만 활용될 요소들을 상수화 한 이유가 있나요?

const router = useRouter();

const handleToggleClick = () => {
setShowPassword(!showPassword);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

이렇게 활용하면 리액트 상태의 비동기성으로 인해 데이터 업데이트 값을 보장하지 못해요

이전 상태값을 수정해야한다면 다음과 같이 활용해주세요

setShowPassword(prev => !prev)

Comment on lines +44 to +55
try {
const response = await axiosInstance.post('/sign-in', data);
if (response.status >= 200 && response.status < 300) {
const accessToken = response.data.data.accessToken;
localStorage.setItem('accessToken', accessToken);
router.push('/folder');
} else {
}
} catch (error) {
setError('email', { message: '이메일을 확인해주세요' });
setError('password', { message: '비밀번호를 확인해주세요' });
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

서비스 로직과 ui로직 분리하기!


const UserInfoContext = createContext<any>(undefined);

export const UserInfoProvider = ({ children }: Props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bk-git-hub

유저 정보만 관리하기보다, auth 관련한 기능을 여기서 관리하도록 해보죠!

@1005hoon 1005hoon changed the base branch from main to part3-김법균 May 22, 2024 16:22
@1005hoon 1005hoon merged commit 6f47749 into codeit-bootcamp-frontend:part3-김법균 May 24, 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