[31기 노규현] 공용 Nav 구현 #6
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/components/nav/Nav.js
Outdated
Comment on lines
6
to
42
| const inners = [ | ||
| { | ||
| id: '1', | ||
| url: '/', | ||
| value: '인기상품', | ||
| }, | ||
| { | ||
| id: '2', | ||
| url: '/', | ||
| value: '신상품', | ||
| }, | ||
| { | ||
| id: '3', | ||
| url: '/', | ||
| value: '온라인 한정사이즈', | ||
| }, | ||
| { | ||
| id: '4', | ||
| url: '/', | ||
| value: '가격 인하', | ||
| }, | ||
| { | ||
| id: '5', | ||
| url: '/', | ||
| value: '기간 한정', | ||
| }, | ||
| { | ||
| id: '6', | ||
| url: '/', | ||
| value: '의복잡화 아울렛', | ||
| }, | ||
| { | ||
| id: '7', | ||
| url: '/', | ||
| value: '생활잡화 아울렛', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
- 이 값 같은 경우에는 매 rendering 마다 새롭게 선언되고 할당될 필요는 없어보입니다.
- 함수형 컴포넌트는 리렌더될 때 마다 함수의 바디부분을 다시 실행합니다.
- 이런 상수데이터를 함수형 컴포넌트 바디 안에서 선언하게 되면 rendering이 일어날때 마다 매번 데이터를 만들고 변수에 값을 할당하는 연산이 일어나게 됩니다.
- 따라서 성능에 악영향을 미칠 수 있습니다.
- 컴포넌트가 사용하는 데이터라고 컴포넌트 안에만 위치할 필요는 없습니다
- 통상 이런 상수데이터같은 경우에는 여러 컴포넌트에서 동시에 사용하는 데이터라면 별도 파일로 분리하거나,이 컴포넌트 안에서만 사용하는 데이터라면 컴포넌트 하단에 작성해줍니다
- 상수데이터의 네이밍은 상수데이터임을 표시하기 위해서 UPPERCASER + snake_case 로 작성해주는게 컨벤션입니다
src/components/nav/Nav.js
Outdated
Comment on lines
44
to
52
| const innerList = inners.map(inner => { | ||
| return ( | ||
| <li key={inner.id}> | ||
| <Link to={inner.url} title={inner.value}> | ||
| {inner.value} | ||
| </Link> | ||
| </li> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
컴포넌트화가 아니고 같은 컴포넌트안에서 변수로 선언하신 이유가 있을까요?
nav컴포넌트 return부분에서는 깔끔해 보여도 연관성을 생각했을 때는 컴포넌트화를 시키거나 return 안에서 map을 돌려주는게 더 연관성 있고 유지보수하기도 좋을 것 같아요!
그리고, 로직이 아닌 단순 리턴이라면 {} 와 return은 생략가능합니다!
Suggested change
| const innerList = inners.map(inner => { | |
| return ( | |
| <li key={inner.id}> | |
| <Link to={inner.url} title={inner.value}> | |
| {inner.value} | |
| </Link> | |
| </li> | |
| ); | |
| }); | |
| const innerList = inners.map(inner => ( | |
| <li key={inner.id}> | |
| <Link to={inner.url} title={inner.value}> | |
| {inner.value} | |
| </Link> | |
| </li> | |
| )); |
src/components/nav/Nav.js
Outdated
Comment on lines
55
to
56
| <nav className="Nav"> | ||
| <div className="NavBox"> |
There was a problem hiding this comment.
컨벤션이 Pascal Case 와 camelCase가 혼용되고 있네요.
팀에서 정한 컨벤션으로 통일시켜 주세요!
src/components/nav/Nav.js
Outdated
| </a> | ||
| <div className="navSearch"> | ||
| <input className="searchInput" type="text" /> | ||
| <img src="/images/nav/searchIcon.png" alt="" /> |
There was a problem hiding this comment.
- img에 alt 값 제대로 부여해주시기 바랍니다! img 태그에 들어가는 alt 속성은 아래와 같은 이유로 매우 중요합니다. (아래와 같은 이유로 alt 속성은 중요하기 때문에 src속성보다 먼저 작성하는게 좋습니다.)
- img에 alt 값 제대로 부여해주시기 바랍니다! img 태그에 들어가는 alt 속성은 아래와 같은 이유로 매우 중요합니다. (아래와 같은 이유로 alt 속성은 중요하기 때문에 src속성보다 먼저 작성하는게 좋습니다.)
이미지가 로딩되지 않을 경우 alt 속성에 적힌 값이 보여짐.
시각장애인의 경우 alt 속성을 통해 어떤 이미지가 보여지는지 알 수 있음.
SEO 검색엔진 최적화에서 어떤 img인지 인식하는 코드는 alt 속성임.
src/components/nav/Nav.js
Outdated
Comment on lines
71
to
73
| <button type="button" className="btnLogin" src="/" /> | ||
| <button type="button" className="btnMypage" src="/" /> | ||
| <button type="button" className="btnCart" src="/" /> |
There was a problem hiding this comment.
- button 태그에는 src속성을 부여할 수 없습니다.
- 버튼 클릭시 이동하는 로직을 구현할려면 onClick 이벤트나 Link 컴포넌트를 이용해서 구현해 주세요!
src/components/nav/Nav.scss
Outdated
Comment on lines
9
to
12
| display: flex; | ||
| justify-content: space-between; | ||
| margin: auto; | ||
| align-items: center; |
There was a problem hiding this comment.
variables.scss 를 잘 활용해주세요!
src/components/nav/Nav.scss
Outdated
Comment on lines
16
to
18
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
variables.scss 를 잘 활용해주세요!
| margin-left: 5px; | ||
| } | ||
| } | ||
| .navSearch { |
There was a problem hiding this comment.
가독성을 위해 셀렉터 시작할 때는 한 줄 띄어주세요!
src/components/nav/Nav.scss
Outdated
| .Nav { | ||
| @include flexCenter; | ||
| justify-items: center; | ||
| width: 100%; |
There was a problem hiding this comment.
- nav는 block 요소입니다.
- 불필요하게 block 요소에
display: block혹은width: 100%속성을 지정하는 것은 피해주세요.
src/components/nav/Nav.scss
Outdated
|
|
||
| .linkItem { | ||
| .btnLogin { | ||
| background: url('../../../public/images/nav/login.png') no-repeat center; |
There was a problem hiding this comment.
- jsx파일에서의 절대경로는 public 폴더가 기준이 됩니다.
- 그리고 css파일에서의 절대경로는 src폴더가 기준이 됩니다.
- 그렇기 때문에 background에서 사용하는 이미지는 src 하위에 assets 폴더를 만들어서 사용하는게 유지보수에 좋습니다.
.btnLogin {
background: url('../../../public/images/Nav/login.png') no-repeat center;
background-size: 23px;
}⬇️ 로 바꿀 수 있습니다.
.btnLogin {
background: url('/assets/login.png') no-repeat center;
background-size: 23px;
}facebook/create-react-app#9937 (comment)
링크 참고해 보세요!
added 2 commits
March 31, 2022 19:04
sodalite1204
reviewed
Apr 1, 2022
src/components/nav/Nav.js
Outdated
| <nav className="nav"> | ||
| <div className="navBox"> | ||
| <div className="navTop"> | ||
| <a href="/" className="Logo"> |
src/components/nav/Nav.js
Outdated
| <img src="/images/nav/searchIcon.png" alt="searchIconImg" /> | ||
| </div> | ||
| <div className="linkItem"> | ||
| <a href="/sign"> |
src/components/nav/Nav.js
Outdated
| <button type="button" className="btnMypage" /> | ||
| </Link> | ||
| <Link to="/"> | ||
| <button type="button" className="btnCart" src="/" /> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
:: 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
:: 구현 목표 (해당 브랜치에서 구현하고자 하는 하나의 목표를 설정합니다.)
공용 페이지에서 고정으로 들어갈 Nav 구현
:: 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
UI 구현
모든 아이콘 이미지에 Link 컴포넌트를 이용하여 링크를 걸어주었다.
컴포넌트 재사용
태그가 달린 nav 아래부분은 추후 추가, 유지보수를 위하여 반복되는 항목을 map메서드를 이용하여 컴포넌트 재사용하였다.
:: 성장 포인트 (해당 기능을 구현하며 고민했던 사항이나 새로 알게된 부분, 어려웠던 점 등을 작성합니다.)
반복되는 항목에 대해 절대 쫄지말고 map 을 신나게 돌려버리자~ 자 돌려라~
Link 컴포넌트와 a 태그의 차이를 한번 더 생각하면서 코드를 작성하자!
:: 기타 질문 및 특이 사항