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

Ryan FE seminar 1 #2

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Ryan FE seminar 1 #2

wants to merge 7 commits into from

Conversation

callasio
Copy link
Collaborator

image

  • "개"를 붙인 이후에는 커서 작동을 직관적으로 만들어야 하는데 이 부분은 제대로 구현하지 못했습니다

@callasio callasio requested a review from wjeongchoi July 23, 2024 14:46
@callasio
Copy link
Collaborator Author

엥 근데 이러니까 zod 세미나 내용까지 같이 commmit되네요

이거 merge하면 다른사람 zod 세미나 내용 때문에 안될 거 같은데 merge는 안하고 남겨놓을까요?

Copy link
Collaborator

@wjeongchoi wjeongchoi left a comment

Choose a reason for hiding this comment

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

(머지 안 할거라서 approve 안 했습니다! 다음 과제도 이 브랜치에서 그대로 작업하고, PR 내용 업데이트해서 re-request 해주세요)

메일 도메인 입력 기능까지 스스로 만들어본 것 아주 좋아요 👍

커서 위치 단위랑 맞추는거 저도 처음에 할 때 못 했어요 쉬운 부분은 아니더라고요 원래 ItemNumberInput 코드 보고 공부해보면 좋을 것 같아요!

코멘트 보고 수정도 해보면 좋을 것 같아요

수고했습니다!!

Comment on lines +1 to +12
export const metadata = {
title: 'Next.js',
description: 'Generated by Next.js',
}

export default function RootLayout({ children }) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거는 왜 생긴 파일인가요?

@@ -0,0 +1,102 @@
/* eslint-disable react/no-children-prop */
Copy link
Collaborator

Choose a reason for hiding this comment

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

실제로 태스크할 때는 eslint disable 최대한 사용하지 말아주세요!

Comment on lines +18 to +23
const MailInput = styled.div`
display: flex;
flex-direction: row;
gap: 10px;
width: 100%;
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

FlexWrapper도 아마 이걸 대체할 수 있을거에요!

Comment on lines +40 to +46
const validateEmailDomain = (domain: string) => {
const domainRegex = /^[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/;
if (!domainRegex.test(domain)) {
return '유효한 메일 도메인을 입력하세요.';
}
return '';
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 이런것도 했네요 좋아요

setIdErrorMessage(validateEmailId(value));
}
}/>
<div style={{ position: 'relative', top: '8px' }}>@</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

웬만하면 div 그대로는 사용하지 말아주세요! 이 경우에는 Typography를 사용할 수 있겠네요

Comment on lines +78 to +86
<Button
style={{height: '36px'}}
children={fixDomain ? "직접 입력" : "kaist.ac.kr"}
onClick={(_) => {
if (!fixDomain) setDomainErrorMessage("");
setMailDomain(fixDomain ? "" : "kaist.ac.kr");
setFixDomain(!fixDomain);
}}
/>
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 +56 to +60
const StyledInputWrapper = styled.div`
display: flex;
flex-direction: column;
gap: 4px;
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

FlexWrapper로 대체 가능!

Comment on lines +20 to +30
const savedNumber = userProfile?.phoneNumber;

const [phoneNumber, setPhoneNumber] = useState(savedNumber ?? "");
const [phoneInput, setPhoneInput] = useState(phoneNumber);
const [phoneError, setPhoneError] = useState(false);

useEffect(() => {
if (userProfile) {
setPhoneNumber(savedNumber ?? "");
}
}, [userProfile, savedNumber]);
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

@wjeongchoi wjeongchoi left a comment

Choose a reason for hiding this comment

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

service 코드 구현까지 잘 되었네요 수고했습니다! 과제 관련해서 추가로 질문 있으면 슬랙 디엠으로 알려주세요~!

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