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

[#99] 로그인 페이지 구현 #102

Merged
merged 9 commits into from
Nov 23, 2023
Merged

Conversation

mahwin
Copy link
Collaborator

@mahwin mahwin commented Nov 23, 2023

한 일

  • fe-dev를 rebase하는 과정에서 타인의 코드가 섞이는 문제가 발생해서 최신 fe-dev에서 새로운 브랜치를 하나 팠습니다.

  • 이전 브랜치
    fe-dev...99-로그인-페이지-구현

  • 로그인 페이지 구현

  • 배포위한 vite 설정 추가

  • 라우팅 작업

  • build 시에 발생하는 타입에러 수정

ToDo

  • accessToken 저장 로직은 [[#69] 로그인 페이지 구현 #69]의 브랜치 코드 그대로 main page가 생기면 추가하겠습니다.
  • 위의 작업은 commons header 작업을 할 때 수행됩니다.

@mahwin mahwin added the FE fe 개발 label Nov 23, 2023
const GITHUB_AUTH_URL = 'http://101.101.208.240:3000/auths/github';

export default function Login() {
const handleLogIn = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In -> in 으로 바꾸주세요 LogIn은 너무 불편한 것이에요

Copy link

Choose a reason for hiding this comment

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

await 가 없는 async 함수네요.

path: '/problem/:id',
element: <ProblemPage />,
},
{ path: '/login', element: <Login /> },
Copy link
Collaborator

Choose a reason for hiding this comment

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

LoginPage로 분리하지 않는 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 일관성을 위해서 Login 컴포넌트를 pages/LoginPage로 만드는 게 좋다고 생각해서 전 pr에는 그렇게 했었는데, 다른 분들은 불편하신 거 같아서 바꿨는데 그대로 진행해도될까요. 저는 라우터에는 page가 붙는 컴포넌트가 있어야 한다고 생각합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

또 다르게 생각하면 컴포넌트를 page로 import하고 그대로 export하는게 더 불편하단 생각도 드는데 어떻게 생각하시나요? 제가 질문에 제대로 이해한 게 맞을까요

Copy link
Collaborator

Choose a reason for hiding this comment

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

제 생각엔 LoginPage를 별도의 페이지로 분리하시려고 했고 그렇다면 pages/LoginPage를 두고 그 안에 Login과 관련된 로직을 둘줄 알았는데 Login컴포넌트를 그냥 넣어서 물어봤습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LoginPage만들어서 Login 로직 분리했습니다!

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

@crongro crongro left a comment

Choose a reason for hiding this comment

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

ㅅㄱㅅㄱ!

@@ -0,0 +1,40 @@
import { css } from '@style/css';

const GITHUB_AUTH_URL = 'http://101.101.208.240:3000/auths/github';
Copy link

Choose a reason for hiding this comment

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

분리한 것 좋네요.

const GITHUB_AUTH_URL = 'http://101.101.208.240:3000/auths/github';

export default function Login() {
const handleLogIn = async () => {
Copy link

Choose a reason for hiding this comment

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

await 가 없는 async 함수네요.

window.location.href = GITHUB_AUTH_URL;
} catch (e) {
const error = e as Error;
console.error(error.message);
Copy link

Choose a reason for hiding this comment

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

개발자에게 알려주기 위해서는 이렇게 console.error
사용자에게는 ?

@mahwin mahwin self-assigned this Nov 23, 2023
Copy link
Collaborator

@dmdmdkdkr dmdmdkdkr left a comment

Choose a reason for hiding this comment

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

LGTM:D

Copy link
Collaborator

@dev2820 dev2820 left a comment

Choose a reason for hiding this comment

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

:)

@mahwin mahwin merged commit d7d7788 into fe-dev Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE fe 개발
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants