-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/soohyeon #2
base: main
Are you sure you want to change the base?
Conversation
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.
수고하셨습니다 수현님! 코드 리뷰 남겨드린 곳 이외에 전체적으로 작성하신 코드 훑어보시면서 리뷰내용 반영할 수 있는 곳은 모두 수정해주세요!
public/images/index.html
Outdated
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="utf-8" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
<meta name="theme-color" content="#000000" /> | ||
<meta | ||
name="description" | ||
content="Web site created using create-react-app" | ||
/> | ||
<title>React App</title> | ||
</head> | ||
<body> | ||
<div id="root"></div> | ||
</body> | ||
</html> |
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 파일은 수정될 필요가 없어보입니다!
처음 clone 받았을때의 버전으로 돌려주세요.
추가적으로 지금 index.html파일의 위치가 public 폴더에 images폴더내부에 위치하고있네요! 위치수정이 필요할 것 같습니다
src/Router.js
Outdated
// import KimCodeLogin from './pages/KimCode/Login/Login'; | ||
// import KimCodeMain from './pages/KimCode/Main/Main'; |
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.
예시로 존재하던 코드는 이제 삭제해주세요!
src/Router.js
Outdated
{/* <Route path="/kimcode-login" element={<KimCodeLogin />} /> | ||
<Route path="/kimcode-main" element={<KimCodeMain />} /> */} |
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.
마찬가지입니다~ 주석은 삭제해주세요
src/Router.js
Outdated
|
||
import Login from './pages/Hwangsoohyeon/Login/Login'; | ||
import Join from './pages/Hwangsoohyeon/Join/Join'; | ||
import Join_ok from './pages/Hwangsoohyeon/Join/Join_ok'; |
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.
컴포넌트 이름은 대문자로 시작하고 camelCase로 작성하는것 지켜주세요~
import Join_ok from './pages/Hwangsoohyeon/Join/Join_ok'; | |
import JoinOk from './pages/Hwangsoohyeon/Join/JoinOk'; |
src/pages/Hwangsoohyeon/Join/Join.js
Outdated
import React, { useState } from 'react'; | ||
import './Join.scss'; | ||
import { useNavigate } from 'react-router-dom'; |
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.
import 순서 컨벤션 가이드 다시 알려드리겠습니다!!
- 모듈
- 컴포넌트
- css파일
순서 생각해서 수정해주세요!
import React, { useState } from 'react'; | |
import './Join.scss'; | |
import { useNavigate } from 'react-router-dom'; | |
import React, { useState } from 'react'; | |
import { useNavigate } from 'react-router-dom'; | |
import './Join.scss'; |
* { | ||
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는 특정파일에 위치하기보단 common.scss파일과같은 전역파일에 작성하는것이 더 좋습니다
select { | ||
color: #999; | ||
font-size: 16px; | ||
font-weight: 400; | ||
border-radius: 6px; | ||
border: 1px solid #e0e0e0; | ||
background-color: #fff; | ||
} |
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.
전체적으로 nesting구조가 이뤄져있지 않다보니 태그를 선택해서 styling하면 의도하지 않은 태그도 해당 속성을 가질 수 있습니다.
scss를 사용하고있는만큼 nesting과 className을 활용한 styling 진행해주세요
}; | ||
|
||
return ( | ||
<div> |
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.
react-intro시간에 말씀드린것 처럼 component의 최상위 태그는 컴포넌트 이름과 같은 className을 가지게 해주세요
const able = inputid.indexOf('@') !== -1 && inputpw.length >= 5; | ||
|
||
return ( | ||
<> |
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.
Fragment는 필요하지 않을 것 같습니다!
.Back { | ||
display: flex; | ||
width: 100%; | ||
height: 56px; | ||
// padding: 8px 497px 8px 0px; | ||
align-items: center; | ||
margin: 24px 0px 0px; | ||
|
||
.Back_arrow { | ||
width: 40px; | ||
height: 40px; | ||
} |
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.
전체적으로 className작성하실 때 camelCase 꼭 지켜주세요
1. 본 PR이 우리 팀의 웹 서비스 제품성에 어떠한 기여를 하였고, 사용자에게 어떠한 기대효과를 전달하는지 작성해주세요.
내 PR이 제품 내 어떠한 기능적인 배경/전후맥락 가운데 개발되었나요?
(예시) 고객의 유입과 전환을 책임지는 회원 기능에서, 유저의 이탈을 방지하기 위해 input 창 바로 아래에서 실시간으로 적색의 경고 메세지를 전달 받습니다.
내 PR이 Merge 됨으로써 유저에게 전달되는 편익/기대효과는 무엇일까요?
(예시) 유저는 실시간으로 스스로 오기입한 내용을 확인할 수 있기 때문에 잘못 계정 정보를 작성함으로써 발생될 수 있는 변수를 최소화할 수 있습니다. 이는 곧 유저 전환의 허들을 낮추어 줄 수 있습니다.
2. 이 브랜치에서 어떤 내용을 개발했는지 큰 제목과 그리고 상세 내역을 적어주세요.
(예시) 로그인 기능 추가
@
필요,password에 숫자로 8자이상 유효성 검사 추가3. 개발한 화면을 캡쳐해서 첨부 해 주세요. (drag & drop 또는 첨부파일 추가)
4. 이 브랜치에서 개발하면서 느꼇던 개발 성장포인트를 적어주세요.
블로그에 정리해봐야겠습니다.