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

[김주동]Sprint1 #12

Merged
merged 1 commit into from
Aug 13, 2024
Merged

[김주동]Sprint1 #12

merged 1 commit into from
Aug 13, 2024

Conversation

joodongkim
Copy link
Collaborator

요구사항

스프린트 미션 디자인(피그마) Sprint1 구현하기

기본

  • index.html 구현
  • style.css 구현

심화

  • index.js 구현

주요 변경사항

스크린샷

image

멘토에게

  • 반응형 HTML 미구현입니다.
  • 디자인 요구사항 미진입니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@joodongkim joodongkim requested a review from GANGYIKIM August 10, 2024 08:27
Copy link
Collaborator

@GANGYIKIM GANGYIKIM left a comment

Choose a reason for hiding this comment

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

주동님 첫 스프린트 미션 수고하셨습니다.
산출물에 대해 걱정하시더니 금방 감을 잡으신 것 같아요.
아래는 전반적인 내용에 관한 코멘트입니다!
다음 미션하실때 참고해주세요


  • 동일한 내용에 대해서는 코드에서 한번만 코멘트를 답니다.
  • git commit 단위를 쪼개시면 좋을 것 같습니다.
    적절한 commit 단위와 메세지를 통해 어떤 작업을 하셨는지 구분할 수 있고, 이를 고려하시면서 작업하셔야 git commit 시점으로 돌아갈때도 도움이 되실겁니다.
  • 클래스 명을 통해 어떤 역할을 하는 태그인지 알 수 있게 이름을 지을때 더 고민해보시면 좋겠습니다.
  • 스프린트 미션 요구사항 중 어떤것은 완수하셨고, 어떤것은 미완인지 PR 올리실 때 같이 올려주시면 좋겠습니다. 이를 보고 코드리뷰할때 참고해서 리뷰를 진행합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
이미지 명에 공백은 들어가지 않는 것이 좋습니다.
또한 어떤 이미지인지 알 수 있는 이름(logo.png)이면 좋을 것 같습니다.

https://www.webauthorings.com/spaces-in-image-names-file-names/

Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
이미지 명들의 규칙이 통일되면 좋을 것 같습니다.


</head>

<body class="desktop">
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
지금으로는 문제가 되지 않지만 추후 반응형작업을 하셔야 하는데
body classname이 desktop인것은 좀 한정적인 이름같습니다.
추후 이름을 변경하시면 좋겠습니다.

<header class="gnb">
<div class="frame_2609410_gnb">
<div class="group_19">
<svg xmlns="http://www.w3.org/2000/svg" width="40" height="41" viewBox="0 0 40 41" fill="none">
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
svg로 로고를 써야하는 이유가 있으셨는지 궁금합니다.
사이즈 변화가 있거나 코드로 색상변경이 필요할때 사용하시는 걸 추천드립니다.

d="M19.6898 32.4992C27.5098 32.4992 33.2427 30.2952 33.2427 24.3659C33.2427 16.9484 27.5098 11.3528 19.6898 11.3528C11.8698 11.3528 6.2666 17.469 6.2666 24.3659C6.2666 30.2952 11.8698 32.4992 19.6898 32.4992ZM19.3823 30.2221C25.1345 30.2221 29.3515 28.6081 29.3515 24.2661C29.3515 18.8342 25.1345 14.7364 19.3823 14.7364C13.6301 14.7364 9.50854 19.2154 9.50854 24.2661C9.50854 28.6081 13.6301 30.2221 19.3823 30.2221Z"
fill="white" />
</svg>
<div id="btnMarket" class="판다마켓logo">판다마켓</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
클래스명은 영어로, 어떤 역할을 하는지 의미있는 이름으로, 일관된 네이밍 규칙에 따라 작성하시는 것을 권장드립니다

d="M19.6898 32.4992C27.5098 32.4992 33.2427 30.2952 33.2427 24.3659C33.2427 16.9484 27.5098 11.3528 19.6898 11.3528C11.8698 11.3528 6.2666 17.469 6.2666 24.3659C6.2666 30.2952 11.8698 32.4992 19.6898 32.4992ZM19.3823 30.2221C25.1345 30.2221 29.3515 28.6081 29.3515 24.2661C29.3515 18.8342 25.1345 14.7364 19.3823 14.7364C13.6301 14.7364 9.50854 19.2154 9.50854 24.2661C9.50854 28.6081 13.6301 30.2221 19.3823 30.2221Z"
fill="white" />
</svg>
<div id="btnMarket" class="판다마켓logo">판다마켓</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
a tag로 작성하시면 페이지 이동을 위한 JS 코드가 불필요합니다.

Suggested change
<div id="btnMarket" class="판다마켓logo">판다마켓</div>
<�a href='/' class="판다마켓logo">판다마켓</a>

Comment on lines +119 to +135
<footer class="frame_33707_footer">
<div class="codeit_2024_footer">
<div>
©codeit - 2024
</div>
<div class="privacy_policy_faq_footer">
<div>Privacy Policy</div>
<div>FAQ</div>
</div>
<div>
<img class="footer-icon" src="img/ic_facebook.png" alt="https://www.facebook.com/">
<img class="footer-icon" src="img/ic_twitter.png" alt="https://twitter.com">
<img class="footer-icon" src="img/ic_youtube.png" alt="https://youtube.com">
<img class="footer-icon" src="img/ic_instagram.png" alt="https://instagram.com">
</div>
</div>
</footer>
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
추후 footer에서 Privacy Policy, FAQ, sns icon들 클릭시 각각에 해당하는 페이지로 이동하게 작업해주세요~


.gnb {
display: flex;
/* width: 1920px; */
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
주석은 필요한 경우만-코드를 이해할때 도움이 되는경우- 남겨주시면 좋겠어요~

color: var(--Primary-Primary-brand, #3692FF);

/* pretendard/2lg-18px-bold */
font-family: Pretendard;
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
pretendard가 첨부되지 않은 상태로 font-family를 적용해주시면 적용되지 않을 가능성이 큽니다.
font를 다운받아 이미지처럼 프로젝트에 넣고 적용해주시거나, google font에서 적용하는 법을 찾아보세요!
다른분들이 어떻게 하셨는지도 보시면 도움이 많이 되실 것 같습니다.

/* pretendard/2xl-24px-medium */
font-family: Pretendard;
font-size: 24px;
font-style: normal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
불필요한 속성은 제거하는게 가독성에 더 좋을 것 같습니다.

@GANGYIKIM GANGYIKIM merged commit 7105e4e into codeit-bootcamp-frontend:Basic-김주동 Aug 13, 2024
@GANGYIKIM
Copy link
Collaborator

다음에 PR 올리실때는 Base-김주동-sprint2로 브런치 만드시고 작업하시는 거 추천드릴께요!
스프린트PR 올리는법 안내를 다시 보시면 좋겠습니다.

https://www.codeit.kr/topics/sprint-fe4-ot/lessons/7108

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