feat : implemented header component #32
Conversation
demo.mov |
SamTheKorean
changed the title
SamTheKorean
changed the title
Screen.Recording.2024-06-11.at.11.42.36.PM.mov최종 데모영상입니다! 디테일한 css는 디자인이 완성되면 다시 수정하겠습니다! |
There was a problem hiding this comment.
@SamTheKorean 님, 제가 sam/23에서 체크아웃해서 웹사이트를 띄워보면 올려주신 데모 영상과 달리 헤더가 잘 표시되지 않는데 무슨 문제일까요?
Shot.2024-06-11.at.18.36.45.mp4
#36의 버그 수정 코드를 이 브랜치에도 동일하게 적용해보니 문제가 해결되네요. 나중에 rebase만 해주시면 될 것 같습니다. 😄 |
|
넵넵 제가 테스트할 때는 버그 수정전에 footer-link 를 주석처리 하고 테스트해서 그렇습니다. 알겠습니다! |
components/header/header.css
Outdated
| header img, | ||
| span { | ||
| vertical-align: middle; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
사실 이게 달레님 해당 포스팅을 참고해서 만든겁니다!! 혹시 잘못된게 있을까요?ㅎㅎ
There was a problem hiding this comment.
ㅎㅎ 이미 보셨군요. 제가 포스팅에 언급하였듯이 vertical-align은 아래와 같은 단점이 있으니, Flexbox를 쓰시면 어떨까요? 로고와 브랜드명 사이에 간격이 일정했으면 좋겠습니다.
There was a problem hiding this comment.
bad practice의 예시였는데 제가 글을 꼼꼼히 안 읽었군요..ㅎㅎ 집어주셔서 감사합니다!! 심지어 이미 부모 속성에서 가운데 정렬이 되어있었는데 제가 불필요하게 추가해놓은 의미없는 코드였네요..!
UI UX 측면에서 버튼의 가로 padding은 새로 padding의 2배 이상으로 맞춰주는 걸 보통 권장합니다. https://app.uxcel.com/courses/ui-components-n-patterns/anatomy-iii-298 |
…egrations with button component
Co-authored-by: Dale Seo <[email protected]>
@sounmind 이 부분은 버튼 컴포넌트를 그대로 쓴 부분이라 Evan님 멘션드립니다! |
SamTheKorean
force-pushed
the
sam/23
branch
2 times, most recently
from
1f5f465
to
48c6300
Compare
|
우선 지금까지 받은 피드백들만 최대한 반영해서 commit했고 반응형 관련 코드도 추가하고 따로 커밋하겠습니다! |
넵 버튼 컴포넌트 내부 스타일링은 신경 쓰지 않고 작업해주시면 됩니다~ |
|
그리고 여기(#42)에 언급된 것처럼 스타일 코드를 JS파일 안에 작성하셨다면 해당 디렉토리에 꼭 담겨 있을 필요가 없겠네요. |
|
디렉토리까지 지우는 게 더 깔끔하겠군요. force push 하겠습니다! |
There was a problem hiding this comment.
소소한 추가 코멘트 드렸지만, @SamTheKorean 님께서 알아서 잘 반영하고 병합하실 거라 믿기 때문에 승인합니다.
components/header.js
Outdated
| } | ||
| </style> | ||
| <header> | ||
| <a href="#" class="logo"> |
There was a problem hiding this comment.
URL을 #로 해도 작동은 하지만, 브라우저 주소줄에 불필요한 #가 붙게 됩니다.
| <a href="#" class="logo"> | |
| <a href="/" class="logo"> |
There was a problem hiding this comment.
refresh를 하지 않고 이동시키는게 목표라 #를 이용했는데 이런 주소줄에 #가 붙는게 많이 안좋긴 하겠군요.
components/header.js
Outdated
| .logo { | ||
| display: flex; | ||
| gap: 10px; | ||
| align-items: center; | ||
| text-decoration: none; | ||
| } | ||
|
|
||
| .logo img { | ||
| width: 45px; | ||
| height: 22.5px; | ||
| } | ||
|
|
||
| .logo span { | ||
| font-size: 20px; | ||
| font-weight: 500; | ||
| color: black; | ||
| } | ||
|
|
||
| .logo img, | ||
| .logo span { | ||
| vertical-align: middle; | ||
| } |
There was a problem hiding this comment.
이 컴포넌트에는 a 요소가 하나 밖에 없는데, 굳이 클래스 선택자를 사용할 이유가 없어 보입니다. 참고로 컴포넌트 기반으로 개발을 하면 이렇게 상대적으로 클래스를 적게 쓰고 되고 따라서 클래스 명명에 대해서 고민할 일도 적어지게 되죠. 소소한 장점입니다.
| .logo { | |
| display: flex; | |
| gap: 10px; | |
| align-items: center; | |
| text-decoration: none; | |
| } | |
| .logo img { | |
| width: 45px; | |
| height: 22.5px; | |
| } | |
| .logo span { | |
| font-size: 20px; | |
| font-weight: 500; | |
| color: black; | |
| } | |
| .logo img, | |
| .logo span { | |
| vertical-align: middle; | |
| } | |
| a { | |
| display: flex; | |
| gap: 10px; | |
| align-items: center; | |
| text-decoration: none; | |
| } | |
| a img { | |
| width: 45px; | |
| height: 22.5px; | |
| } | |
| a span { | |
| font-size: 20px; | |
| font-weight: 500; | |
| color: black; | |
| } | |
| a img, | |
| a span { | |
| vertical-align: middle; | |
| } |
There was a problem hiding this comment.
사실 저도 이 부분을 a로 할 지 logo로 할 지 고민하다가 class를 사용하면 각 요소의 역할을 구체적으로 명명하는 느낌이라 가독성?이 늘거라고 판단해 logo 클라스를 생성했는데 코멘트를 보고 생각해보니 덜 쓰는게 좋고 best practice 인 것 같네요ㅎㅎ 피드백 감사합니다! 굳이 class 명명을 안해도 되는 게 컴포넌트의 장점 중 하나이군요:)
There was a problem hiding this comment.
이렇게 자바스크립트 파일 하나로 뽑아내니, 생각했던 것보다 더 심플하고 좋은데요? At least, for me lol
Related Issue
Changes Made