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

SKRF-111 design: Login Page 디자인 #10

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

colorkite10
Copy link
Collaborator

📝요구사항과 구현내용

로그인 페이지 뷰를 구현했습니다.

구현 스크린샷

image

✨pr포인트 & 궁금한 점

  • 로고를 공통 컴포넌트로 만드는 게 좋을까요?
  • 별 다른 내용은 없지만 컨벤션은 잘 지켜졌는지, 코드는 어떻게 작성하면 좋을지 편하게 코멘트 달아주시면 감사하겠습니다! 🙇‍♂️🙇‍♂️🙇‍♂️

@colorkite10 colorkite10 self-assigned this Oct 23, 2023
@colorkite10 colorkite10 changed the title design: Login Page 디자인 SKRF-111 design: Login Page 디자인 Oct 23, 2023
Copy link
Collaborator

@hyesung99 hyesung99 left a comment

Choose a reason for hiding this comment

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

^.^b 리뷰 참고해서 몇가지만 수정 부탁드립니다!

Comment on lines 41 to 44
font-style: normal;
font-family: 'LogoFont';
font-weight: 400;
line-height: normal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
normal보다는 rem을 통해 구체적인 수치를 지정해주는게 좋다고 생각하는데 어떠신가요?.?

Comment on lines 65 to 72
export {
ContainerStyled,
LogoAreaStyled,
LogoCircleStyled,
LogoTextStyled,
LoginAreaStyled,
TitleStyled,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

p1;
이건 아직 정해지지 않은 컨벤션인데
저는 스타일 변수는 각각 export하는데 모아서 export하는걸로 통일할까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ㅋㅋ;

import { LogoText, Message } from '@/constants/LoginPage';

import {
ContainerStyled,
Copy link
Collaborator

Choose a reason for hiding this comment

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

p2;
무엇을 위한 컨테이너인지 표시해주시면 좋을것 같아요!
ex) LogoContainerStyled

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 17 to 23
<LogoTextStyled>
<span>{LogoText.SPACE_CLUB}</span>
</LogoTextStyled>
</LogoCircleStyled>
</LogoAreaStyled>
<LoginAreaStyled>
<TitleStyled>{Message.WELCOME}</TitleStyled>
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
TitleStyled는 내부에 바로 텍스트가 들어가는데
LogoTextStyled는 span안에 텍스트를 넣으신 이유가 따로 있으신가요?
span에 스타일이 적용되지 않았다면 없어도 되지 않을까?하는 생각이 드네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇네요! 감사합니다 !

<LogoAreaStyled>
<LogoCircleStyled>
<LogoTextStyled>
<span>{LogoText.SPACE_CLUB}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

p2;
이런 제목 느낌의 중요한 텍스트 h1태그로 감싸줄때 구글 SEO최적화에 좋습니다!

</LogoCircleStyled>
</LogoAreaStyled>
<LoginAreaStyled>
<TitleStyled>{Message.WELCOME}</TitleStyled>
Copy link
Collaborator

Choose a reason for hiding this comment

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

p2;
TitleStyled도 h1으로 감싸기!

font-family: 'LogoFont';
font-weight: 400;
line-height: normal;
`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

font-style, font-weight 적용되는지 확인 부탁드립니당:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

확인 결과 적용 안 됩니다...!

import { LogoText, Message } from '@/constants/LoginPage';

import {
ContainerStyled,
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거는 그대로 가시나요?

@colorkite10 colorkite10 merged commit 838b2e9 into main Oct 24, 2023
@colorkite10 colorkite10 deleted the SKRF-111-design-login-page branch October 24, 2023 07:18
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.

3 participants