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

[박준환] sprint5 #212

Conversation

park521
Copy link
Collaborator

@park521 park521 commented Nov 24, 2024

Github에 PR(Pull Request)을 만들어서 미션을 제출합니다.
피그마 디자인에 맞게 페이지를 만들어 주세요.
React를 사용합니다

스크린샷

image

멘토에게

미완성 제출하겠습니다.

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@park521 park521 requested a review from devToram November 24, 2024 14:04
@park521 park521 self-assigned this Nov 24, 2024
Copy link
Collaborator

@devToram devToram left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 :)

</div>
<div>
<a href="https://ko-kr.facebook.com/">
<img src="./img/Vector.png">
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지에 alt 값 넣어주세요!


//Email Validation
function validateEmail(email) {
const regex = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

정규표현식의 경우 따로 상수화 시켜주시지 않으면 해당 함수가 실행될 때마다 상수가 선언되어요!
밖으로 빼주시는 것을 추천드립니다!

Suggested change
const regex = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+/;
const REGEX = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+/;


//Password Validation
function validatePassword(password) {
return password.length >= 8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

8 이라는 상수값도 따로 어떤 수인지를 알기 쉽게 상수로 정의해주시면 좋을 거 같아요!

Suggested change
return password.length >= 8;
const MINIMUM_PW_LENGTH = 8
....
return password.length >= MINIMUM_PW_LENGTH;

Comment on lines +21 to +25
emailInput.classList.add("login-form__input--error");
emailError = document.createElement('p');
emailError.textContent = '잘못된 이메일 형식입니다.';
emailError.classList.add('login-form__input--error-message');
emailInput.parentElement.appendChild(emailError);
Copy link
Collaborator

Choose a reason for hiding this comment

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

실제 DOM 을 추가하는 방식은 sideEffect 가 많을 수 있어서 className 으로 속성을 제어하는 방식도 고려해보심 좋을 거 같아요!

}

// Create Passweord Error Message
function passwordError(input) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

위의 emailError 와 공통된 부분이 많아 공통화해보시면 좋을 거 같습니다!

const [pageSize, setPageSize] = useState("10");
const [itemList, setItemList] = useState([]);
const [keyword, setKeyword] = useState("");
const [totalPageNum, setTotalPageNum] = useState();
Copy link
Collaborator

Choose a reason for hiding this comment

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

num 인데 초기값이 undefined 네요~ 의도하신 게 아니라면 number 값을 지정해주세요!

Comment on lines +14 to +16
const handleNewestClick = () => setOrderBy("recent");

const handleFavoriteClick = () => setOrderBy("favorite");
Copy link
Collaborator

Choose a reason for hiding this comment

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

하나의 함수로 만들고 파라미터로 값을 받는 방법을 쓰면 중복이 줄 거 같아요!

Suggested change
const handleNewestClick = () => setOrderBy("recent");
const handleFavoriteClick = () => setOrderBy("favorite");
const handle�OrderChangeClick = (chosenOrder) => setOrderBy(chosenOrder);

Comment on lines +29 to +38
const getPageSize = () => {
const width = window.innerWidth;
if (width < 768) {
return 4;
} else if (width < 1280) {
return 6;
} else {
return 10;
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

상수값들을 쓰실 때는 const 로 빼주셔서 의미를 나타내시거나 주석으로 어떤 값인지 적어주시면 이해하기 좋을 거 같아요!

</div>
</div>
<div className="allItemsCardSection">
{itemList?.map((item) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

? 좋네요 👍

const [itemList, setItemList] = useState([]);
const [pageSize, setPageSize] = useState("4");

const getPageSize = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

중복되는 코드들은 util 화해서 공통화해보심 좋을 거 같아요!

@devToram devToram merged commit 9162d83 into codeit-bootcamp-frontend:React-박준환 Nov 26, 2024
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.

4 participants