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

[7주차] 비트버디 미션 제출합니다. #4

Open
wants to merge 87 commits into
base: master
Choose a base branch
from

Conversation

ddhelop
Copy link
Member

@ddhelop ddhelop commented Jun 26, 2024

배포주소

BB-vote

동혁 코멘트

비트버디 웹 개발에 대한 모의개발이라고 생각해서, 똑같은 뷰와 환경으로 과제를 진행해봤습니다. 과제를 진행하면서 팀원과의 협업 스타일 및 본 개발 시작시 발생할 문제점에 대해서 파악하는데 중점으로 두었고, 백엔드 팀원과 소통방식에 대해서도 확인하는 정말 좋은 과제였던 것 같습니다.
이번 과제에서 이슈들이 많았는데, 이를 보완해서 본 개발 때 완성도있는 결과물을 만들어 보겠습니다~!

수현 코멘트

백엔드 팀원들이 늦은 새벽에도 오류 확인해주시고 해결해주셔서 같이 과제하는 동안 엄청 든든했습니다! 비트버디 개발이 기대되는 협업이었어요 🤩 이번이 벌써 마지막이라는게 믿어지진 않지만 …. 😿 마지막 과제도 수고 많으셨습니다 ☺️

디자인

협동과제 피그마 디자인의 UI에 비트버디 스타일을 입혀서 디자인 작업을 해보았습니다. 좀 더 신경을 써야할 게 많아서 시간이 더 걸렸지만 결과물이 좋아서 만족합니다~!

백엔드 협업

생전 처음보는 403에러, CORS block, swagger 문서 등등 다양한 문제점들을 마주쳤지만, 새벽 4시에 help 요청을 보내도 바로 답장해주는 팀원덕분에 빠르게 에러를 해결할 수 있었습니다.
TMI - 자신의 도메인까지 가져와서 https 통신까지 해줌...

구현화면

image image

ddhelop and others added 30 commits May 23, 2024 18:22
Copy link

@songess songess left a comment

Choose a reason for hiding this comment

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

마지막과제 고생하셨습니다!
api 호출 함수들이 깔끔하게 정리되어 있고 데이터와 타입들도 잘 분리되어 있어 리뷰를 편하게 할 수 있었어요.
리팩토링을 하게되면 로그인하지 않은 유저들도 투표페이지/투표결과 페이지에 접근했을 때를 고려해보면 좋을 거 같아요!

er.name Outdated
Copy link

Choose a reason for hiding this comment

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

이 파일은 뭔가요?

Copy link

Choose a reason for hiding this comment

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

ㅠㅠ

'./src/components/**/*.{js,ts,jsx,tsx,mdx}',
'./src/app/**/*.{js,ts,jsx,tsx,mdx}',
],
theme: {
Copy link

Choose a reason for hiding this comment

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

theme 설정 좋아요!

Comment on lines +74 to +77
index: 9,
name: '송은수',
team: 'TIG',
},
Copy link

Choose a reason for hiding this comment

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

index 9..!

Comment on lines +10 to +20
const [auth, setAuthState] = useRecoilState(authState);

useEffect(() => {
const loadAuthState = () => {
const storedAuth = localStorage.getItem('authState');
if (storedAuth) {
setAuthState(JSON.parse(storedAuth));
}
};
loadAuthState();
}, [setAuthState]);
Copy link

Choose a reason for hiding this comment

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

recoil을 사용해 전역상태로도 사용하고 localstorage에도 저장한 이유가 있을까요? localstorage만 사용해도 되지 않을까 싶어서요.

Comment on lines +22 to +28
const handleLogout = () => {
setAuthState({
isLoggedIn: false,
username: '',
});
localStorage.removeItem('authState');
};
Copy link

Choose a reason for hiding this comment

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

로그아웃시 authState만 지워줘서 토큰이 그대로 남아있어요. 그에따라 다른페이지에서 예상치 못한 동작들이 발생하는 거 같아요(ex. 로그인 하지 않고도 투표가능).

const [candidateList, setCandidateList] = useState<Candidate[]>([]);
const router = useRouter();

const onClickVote = async (memberId: number) => {
Copy link

@songess songess Jun 27, 2024

Choose a reason for hiding this comment

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

본인을 투표할 수 없도록 검사하는 로직이 필요할 거 같고 한 번 투표를 했다면 더 이상 투표를 못하게 막는 로직도 필요할 거 같아요!

또한 현재 로그인을 한 번이상 하고 로그아웃 -> 이 페이지에 URL을 쳐서 들어오면 투표를 할 수 있어요.

localStorage.setItem('token', result.token);
router.push('/');
} catch (error) {
console.error('로그인 실패', error);
Copy link

Choose a reason for hiding this comment

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

실패알림이 console.log가 아니라 alert이면 좋을 거 같아요.

Choose a reason for hiding this comment

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

네, 현재 배포하신 것을 기준으로 옳지 못한 계정 정보를 통해 로그인할 때, 아무런 UI가 나타나지 않아서 alert 창이나 모달을 띄워주셔도 좋을 것 같습니다.

className={`flex w-full cursor-pointer items-center justify-between rounded-lg border-2 border-main px-6 py-3 text-center hover:bg-main active:bg-main ${index === 0 ? 'bg-main' : ''}`}>
<div className="flex items-center gap-6">
<div className="rounded-lg bg-white">
<p className="px-3 text-xl font-semibold text-BG-black">{index + 1}</p>
Copy link

Choose a reason for hiding this comment

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

공동순위 로직이 있었으면 좋았을 거 같아요.

추가적으로 두자릿수가 되었을 때 칸이 살짝 밀리는데, flex-basis로 기본 크기를 지정하는 방법 등으로 해결할 수 있을 거 같아요.

{candidateList?.map((result, index) => (
<div
key={result.id}
className={`flex w-full cursor-pointer items-center justify-between rounded-lg border-2 border-main px-6 py-3 text-center hover:bg-main active:bg-main ${index === 0 ? 'bg-main' : ''}`}>
Copy link

Choose a reason for hiding this comment

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

hover시 효과가 있는데 그것때문에 뭔가 기능이 있어야 할 거 같은데 없어서 이질감?이 들어요!

const accessToken = localStorage.getItem('token') || '';
try {
const response = await candiateList(accessToken);
console.log('투표 결과:', response);
Copy link

Choose a reason for hiding this comment

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

console.log가 아직 남아있어요!

Copy link

@Programming-Seungwan Programming-Seungwan left a comment

Choose a reason for hiding this comment

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

늘 그랬듯이 비트버디 팀은 UI도 이쁘게 정말 잘 짜시고, 이번에 useForm 커스텀 훅을 이용하시는 모습에서 정말 많이 배울 수 있었습니다!

이번 과제도 정말 고생 많으셨습니다 :)

Comment on lines +10 to +23
export const metadata: Metadata = {
title: 'beatBuddy vote',
description: '19th beatBuddy vote',
icons: {
icon: '/logo.png',
},
};

const pretendard = localFont({
src: '../lib/fonts/PretendardVariable.woff2',
display: 'swap',
weight: '45 920',
variable: '--font-pretendard',
});

Choose a reason for hiding this comment

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

루트 레이아웃 페이지에서 공통적으로 가져갈 메타 데이터들 설정해주신 것 👍

<body className={`${pretendard.className}`}>
<RecoilProvider>
{/* Full Container */}
<div className="flex h-full w-full items-center justify-center">

Choose a reason for hiding this comment

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

태그명을 main으로 잡는 것이 추후에 프로젝트를 이해하는 데에 더 좋을 것 같아요

const router = useRouter();
const setAuthState = useSetRecoilState(authState);
const { values, errors, touched, handleChange, handleBlur, handleSubmit, setFieldValue } = useForm<LoginFormData>({
initialValues: initialLoginValues,

Choose a reason for hiding this comment

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

initialValues라는 값은 lib 디렉터리의 type 파일이 아니라, 현재 파일에서 선언해주는 것이 더 낫다고 생각합니다

return errors;
};

export const LoginValidation = (values: Partial<FormData>) => {

Choose a reason for hiding this comment

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

데이터가 들어와서 에러가 있다면 해당 에러를 반환하는 구분된 로직 좋은 것 같습니다

Comment on lines +40 to +50
const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
setIsLoading(true);
const validationErrors = validate(values);
setErrors(validationErrors);

if (!Object.keys(validationErrors).length) {
await onSubmit(values);
}
setIsLoading(false);
};

Choose a reason for hiding this comment

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

사용자가 제출을 눌렀을 떄, 유효성 검사를 프론트엔드 단에서 먼저 진행하고 에러가 없는 상태라면 실제 onSubmit 전송 로직이 깔끔한 것 같네요

Copy link

Choose a reason for hiding this comment

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

네 저희도 백에서 구현해줘서 크게 신경 안쓰고 있는 부분이었는데 좋은거 같습니다!

if (!values.name) {
errors.name = '사용하실 이름을 입력해주세요.';
} else if (!/^[가-힣a-zA-Z][^\s]{0,9}$/.test(values.name)) {
errors.name = '한글이나 영어로 시작하는 10자 이내의 이름이어야 해요.';

Choose a reason for hiding this comment

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

현재 회원가입 페이지에서 한글 이름을 ㅁㄴㅇㄹㄴㅇ 과 같이 입력했을 떄에도 오류 메시지가 나타나는 것 같습니다. 정규 표현식을 이용한 로직의 확인 한번 부탁드립니다.

Choose a reason for hiding this comment

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

맞아요. 그리고 그냥 /vote 페이지를 url 입력해서 접근했을 떄 현재 404 에러가 뜨는데 이럴 때에도 /vote/select 페이지로 리다이렉트 시켜주면 좋을 것 같아요

Comment on lines +17 to +20
} else if (response.status === 400) {
alert(data.message);
}
} catch (error) {}

Choose a reason for hiding this comment

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

현재의 로직은 try ~ catch 문의 특성을 온전히 활용하고 있지 못한 것 같아요. 아예 else if 문에서 내에서 response의 status에 맞게 다시 한번 분기처리해서 각각 그 내용에 맞게 throw new Error() 문을 부여한 뒤, 다양한 catch 문을 만드시고 거기에서 alert 하여 좀더 친근한 사용자 오류를 보여줄 수 있으면 좋을 것 같아요. 물론 모달로 보여주는 방식은 더 훌륭하구요!

const router = useRouter();

const onClickVote = async (teamId: number) => {
const token = localStorage.getItem('token') || '';

Choose a reason for hiding this comment

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

현재 로그인 안된 사용자가 https://react-vote-19th-two.vercel.app/demo-day-vote/vote 주소로 무단 접속해서 한 팀을 투표할 떄 그냥 투표가 완료되었다는 alert 메시지가 나오는 것 같아요. 일단 제가 확인한바로 결과에 반영은 안되는데(이런 거 변태처럼 확인하는 스타일이라 죄송합니다 😂), 로컬 스토리지에서 토큰을 깠을때 지금처럼 '' 같은 로직이 아니라 그냥 null 값이 나올텐데, 이걸 바로 throw new Error("로그인되지 않은 사용자입니다")처럼 에러로 던지고 catch 문에서 해당 오류를 alert해서 보여주면 더 좋을 것 같아요

"lint": "next lint"
},
"dependencies": {
"axios": "^1.7.2",

Choose a reason for hiding this comment

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

현재 프로젝트 전체적으로 axios 모듈을 사용하고 있지 않은 것 같아요. 지워도 될 것 같습니다.

Copy link

@Dahn12 Dahn12 left a comment

Choose a reason for hiding this comment

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

이후 구현할 프로젝트에 맞춰서 만들 생각은 못했는데 비트버디도 기대됩니다! 마지막 스터디 수고많았어👍

Comment on lines +20 to +42
const { values, errors, touched, handleChange, handleBlur, handleSubmit, setFieldValue } = useForm<FormData>({
initialValues: initialFormData,
onSubmit: async (values) => {
try {
const response = await signUpRequest(values);
const result = await response.json();
if (response.ok) {
console.log('회원가입 성공:', result);
router.push('/login');
} else {
throw new Error(result.message || '회원가입 실패');
}
} catch (error) {
if (error instanceof Error) {
setModalMessage(error.message);
} else {
setModalMessage('알 수 없는 오류가 발생했습니다.');
}
setModalOpen(true);
}
},
validate: SignUpValidation,
});
Copy link

Choose a reason for hiding this comment

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

저도 배워갑니다...!

Comment on lines +29 to +34
<div
onClick={() => onClickVote(1)}
className="flex h-20 w-40 cursor-pointer flex-col items-center justify-center gap-1 rounded-lg border-2 border-main bg-BG-black text-center hover:bg-main active:bg-main">
<p className="text-xl font-semibold">비트버디</p>
<p className="text-xs text-gray-400">베뉴 큐레이팅 서비스</p>
</div>
Copy link

Choose a reason for hiding this comment

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

각각을 컴포넌트로 빼면 더 깔끔했을거 같아요...!


<div className="flex w-full gap-12">
<div className="flex w-full flex-col gap-6">
{candidateList?.map((result, index) => (
Copy link

Choose a reason for hiding this comment

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

후보자 유무를 확인해 준 부분도 사소하지만 좋아요!

Comment on lines +40 to +50
const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
setIsLoading(true);
const validationErrors = validate(values);
setErrors(validationErrors);

if (!Object.keys(validationErrors).length) {
await onSubmit(values);
}
setIsLoading(false);
};
Copy link

Choose a reason for hiding this comment

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

네 저희도 백에서 구현해줘서 크게 신경 안쓰고 있는 부분이었는데 좋은거 같습니다!

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.

5 participants