-
Notifications
You must be signed in to change notification settings - Fork 39
[최재호] sprint1 #39
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 #39
The head ref may contain hidden characters: "Basic-\uCD5C\uC7AC\uD638"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
전체적으로 클래스이름 만들때 개선할 여지가 많이 보이는것같아요.
다음 미션부터는 드린 피드백 반영해보시면서 좀 더 다듬어봅시다 :)
주요 리뷰 포인트
- 클래스이름 수정
- 네이밍케이스, 네이밍컨벤션
- 스타일 중복, 코드 중복 줄이기
index.html
Outdated
| @@ -0,0 +1,149 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="kr"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 조금 헷갈리죠 ㅎㅎ kr이 아니라 ko로 변경해주셔야 올바른 문법입니다!
| <html lang="kr"> | |
| <html lang="ko"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 기억하도록 하겠습니다. 감사합니다.
index.html
Outdated
| <a href="index.html" | ||
| ><img | ||
| class="logo-img" | ||
| src="./logo.png" | ||
| alt="판다마켓 로고" | ||
| /></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근성 향상을 위해 aria-label="홈으로 이동" 과 같이 관련있는 요소에 aria-label 혹은 role을 사용해볼까요?
아래 아티클 참고해보시고 접근성을 향상시키기 위해 어떤 노력을 할수있는지 더 찾아보시면 좋은 공부 될 것같아요 :)
https://joyhong-91.tistory.com/22
https://velog.io/@a_in/WAI-ARIA-role-aria-label
index.html
Outdated
| </header> | ||
|
|
||
| <main> | ||
| <section class="section1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아까 얘기한것과 동일한 이유로, section의 순서가 바뀌면 네이밍도 다 변경해야겠죠? 요것도 변경해주세요 :)
index.html
Outdated
| <main> | ||
| <section class="section1"> | ||
| <div class="section1-div"> | ||
| <div class="part1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도!
개선해야하는 이유와 배경을 이해하셨다면 다른 부분은 코멘트 안드려도 아시겠죠? :)
index.html
Outdated
| <a href="./faq.html" class="FAQ">FAQ</a> | ||
| </div> | ||
| <div class="social-icon button"> | ||
| <a href="https://www.facebook.com/?locale=ko_KR" target="_blank"><img src="./icon_facebook.png" alt="facebook link" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
외부 링크의 경우, 보안을 위해 rel="noopener noreferrer"를 추가해주세요! :)
noopener: 새 탭에서 열린 페이지가 원본 페이지에 접근하는 것을 방지
noreferrer: 리퍼러 정보가 새 페이지로 전달되는 것을 방지
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앞으로 외부 링크인 경우에는 rel="noopener noreferrer"를 추가하도록 하겠습니다.
style.css
Outdated
| * { | ||
| margin: 0 0; | ||
| padding: 0 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
용도를 나눠 css 파일을 관리해볼까요?
- reset.css (css 초기화)
- common.css (전역 스타일 처리)
style.css
Outdated
| a.button { | ||
| text-decoration: none; | ||
| color: #ffffff; | ||
| background-color: #3692ff; | ||
| padding: 10px 20px; | ||
| border-radius: 10px; | ||
| } | ||
|
|
||
| .button { | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러 곳에서 사용되는 공통된 스타일 규칙들을 추출해서 가장 기본이 되는 버튼 클래스를 만들고, modifier를 사용해 변형된 스타일을 추가하시는게 좀 더 확장성있게 개발하실수 있으면서도, 유지보수에 유리하고 코드 중복도 줄일 수 있을거예요.
아래 방식으로 리팩토링 해보시는걸 추천드려봅니다!
.btn {
cursor: pointer;
display: inline-block;
text-decoration: none;
text-align: center;
border: none;
background-color: #3692ff;
color: #ffffff;
padding: 10px 20px;
border-radius: 10px;
font-size: 16px;
transition: background-color 0.2s ease;
}
.btn--rounded {
border-radius: 50px;
}
.btn--small {
padding: 8px 16px;
font-size: 14px;
}
.btn--large {
padding: 12px 24px;
font-size: 18px;
}
.btn:hover {
background-color: #2b74cc; /* Darker shade for hover */
}
.btn:active {
transform: translateY(1px);
}
.btn:focus {
outline: 2px solid #3692ff;
outline-offset: 2px;
}
.btn--primary {
background-color: #3692ff;
color: #ffffff;
}
.btn--secondary {
background-color: transparent;
color: #3692ff;
border: 2px solid #3692ff;
}
.btn--secondary:hover {
background-color: #3692ff;
color: #ffffff;
}| } | ||
|
|
||
| .section1 { | ||
| background-color: #cfe5ff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자주 쓰이는 값들은 css 변수로 관리해서 재사용해볼까요?
이렇게 해주면 마찬가지로 코드 중복을 줄이면서도 유지보수에 유리한 방향으로 코드를 개선할 수 있어요 :)
예시)
/* Variables */
:root {
--primary-color: #3692ff;
--text-primary: #374151;
--text-secondary: #9ca3af;
--text-light: #e5e7eb;
--background-primary: #fcfcfc;
--background-secondary: #cfe5ff;
--background-dark: #111827;
--border-radius-sm: 8px;
--border-radius-lg: 12px;
--border-radius-xl: 40px;
--container-width: 1120px;
--content-width: 988px;
}
style.css
Outdated
| .section1-img { | ||
| height: 250px; | ||
| width: auto; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다음 미션부터는 포맷팅도 신경써봐요 :)
불필요한 공백은 없애볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 알겠습니다. 포멧팅도 신경쓰도록 하겠습니다.
질문에 대한 답변
네! 질문하신 사항 모두 PR 본문 내에 코멘트로 답변해드렸습니다 :) |
Alex-Choi0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단한것 먼저 반영했습니다.
요구사항
기본
심화
스크린샷
배포한 사이트 링크
링크
멘토에게
@addiescode-sj