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

Next.js 이동규 sprint5 #719

Open
wants to merge 17 commits into
base: Next.js-이동규
Choose a base branch
from

Conversation

ldkstellar
Copy link
Collaborator

요구사항

기본

  • 리액트훅폼으로 회원가입페이지 만들기
  • 무한스크롤 구현하기

주요 변경사항

-리액트 훅폼사용

  • 무한스크롤 구현

스크린샷

멘토에게

  • 무한스크롤 구현이렇게 하면 될까요?
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@ldkstellar ldkstellar requested a review from endmoseung July 12, 2024 14:37
Copy link
Collaborator

@endmoseung endmoseung left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 로직은 잘 짜셨는데 훅으로 나누거나 코드를 정리하는것만 된다면 더 완벽한 코드가 될 것 같네요 파트3동안 수고하셨습니다:)

Comment on lines 2 to 3
type I = HTMLInputElement;
type D = HTMLDivElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입이름을 더 명확하게 지었으면 좋겠습니다!
이러면 나중에 알아보기가 힘들어요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@endmoseung 리액트훅폼으로 리펙토링했습니다ㅜㅜ

Comment on lines 6 to 11
let email = inputBox.value.trim();
if (!isValidEmail(email)) {
showError('잘못된 이메일입니다', inputBox, errorBox);
} else {
clearError(inputBox, errorBox);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

let을 쓰지 않아도 구현 가능할 것 같습니다!

Comment on lines 30 to 36

export function isValidEmail(email: string) {
// 이메일 형식을 정규표현식을 사용하여 검사
let emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
return emailRegex.test(email);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

여기두요

Comment on lines 24 to 33
useEffect(() => {
const observer = new IntersectionObserver(callback, { threshold: 1 });
if (target.current !== null) {
observer.observe(target.current);
}
return () => {
observer.disconnect();
};
}, []);

Copy link
Collaborator

Choose a reason for hiding this comment

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

이부분은 훅으로 뺼 수 있겟네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 알갰습니다.

Comment on lines +8 to +18
export interface SignUp {
email: string;
nickName: string;
password: string;
repeatPassword: string;
}

export interface Show {
passShow: boolean;
repeatShow: boolean;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

api는 파일로 따로 관리하면 추후 재사용성이 증가해요!

Comment on lines +38 to +40
useEffect(() => {
saveComment();
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
useEffect(() => {
saveComment();
}, []);
useEffect(() => {
if(id){
saveComment();
}
}, []);

위처럼 바꾸시면 함수에 타입단언 쓰지 않아도 될 것 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 12 to 17
useEffect(() => {
if (!ACCESS_TOKEN) {
alert('로그인이 필요합니다.');
routes.replace('/boards');
}
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

코드의 가독성을 위해서 effect의 위치는 아래측에 위치하면 통일감 있겠네요:)

src/api/axios.ts Outdated
(config: InternalAxiosRequestConfig<any>) => {
const token = Cookies.get('token');
(config) => {
const token = Cookies.get('accessToken');
Copy link
Collaborator

Choose a reason for hiding this comment

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

쿠키에 들어가는 키값들도 따로 상수로 관리하시면 실수 할 여지도 줄어들고 추후에 유지보수에 도움이 됩니다!

{
refreshToken: Cookies.get('refreshToken'),
},
{ headers: { 'Content-Type': 'application/json' } }
Copy link
Collaborator

Choose a reason for hiding this comment

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

요 값은 axiosinstance에서 고정적으로 심어두면 좋아요. 왠만하면 content-type은 고정이라서요!

const [posts, setPosts] = useState<writingType[]>([]);
const target = useRef<HTMLDivElement>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

target보다는 scrollRef같은것으로 무한스크롤 이름 주눈건 어떨까요??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants