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

[김태훈] sprint4 #125

Conversation

horororok
Copy link
Collaborator

요구사항

기본

  • [x]
  • [x]
  • [x]

심화

  • [x]
  • [x]

주요 변경사항

  • signin.js
  • singup.js

스크린샷

image

멘토에게

  • 보일러 플레이트를 줄이기 위해 공통함수를 만들어 보았는데 해당 방법이 유효한 건지 혹은 더 좋은 방법이 있는지 궁금합니다.

@horororok horororok requested a review from baeggmin September 27, 2024 09:58
@horororok horororok added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Sep 27, 2024
@horororok horororok changed the title sprint4 완료 [김태훈] sprint4 Sep 27, 2024
@horororok horororok requested review from lang92 and removed request for baeggmin October 1, 2024 04:42
Copy link
Collaborator

@lang92 lang92 left a comment

Choose a reason for hiding this comment

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

js를 정말 잘 활용해주셨네요 태훈님! 👏👏👏 3박수 드립니다.

중복되는 리뷰는 드리지 않았으니 참고해서 리뷰 남기지 않은 다른 부분들까지 수정해 주세요!

Comment on lines +29 to +31
<a href="/"
><img src="images/logo/logo.svg" alt="판다마켓 홈" width="153"
/></a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

요소의 attribute로도 스타일을 줄 수 있지만, 일반적으로 스타일은 css로 적용하는 게 유지보수에 좋습니다.
요소의 style 속성, 이외에 width같은 attribute, 별도의 css 파일, <style> 요소 등으로 다양하게 스타일을 줄 수 있지만 여러 곳에 산재돼 있으면 관리하기 어려우니까요!
이후에 스타일 작업하실 때 참고해 주세요!

Comment on lines +38 to +54
<h1>
일상의 모든 물건을<br />
거래해 보세요
</h1>
<a href="items.html" class="button pill-button">구경하러 가기</a>
</div>
</section>

<section id="features" class="wrapper">
<div class="feature">
<img src="images/home/feature1-image.png" alt="인기 상품" />
<div class="feature-content">
<h2>Hot item</h2>
<h1>
인기 상품을 <span class="break-on-desktop"><br /></span>확인해
보세요
</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

h1, h2 태그와 같은 heading 태그를 사용할 땐 권장되는 사용 방식들이 있습니다. 참고해 주세요!
image

<div class="feature-content">
<h2>Hot item</h2>
<h1>
인기 상품을 <span class="break-on-desktop"><br /></span>확인해
Copy link
Collaborator

Choose a reason for hiding this comment

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

span으로 감쌀 필요 없이 br 태그만 사용해도 될 것 같습니다.

</div>
</section>

<section id="features" class="wrapper">
Copy link
Collaborator

Choose a reason for hiding this comment

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

반드시 필요한 경우(label 태그와 input 태그, a 태그 사용 시 활용 등)가 아니라면 id 속성의 사용은 지양해 주세요. id는 문서 전체에서 유니크해야 하기 때문에 네이밍할 때 번거롭고 재사용하기도 힘드니까요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

특히 나중에 react에서는 컴포넌트를 재사용하기 때문에 더욱 id의 사용에 유의하셔야 합니다.

Comment on lines +16 to +25
<style>
.error-message {
color: red;
font-size: 0.8em;
margin-top: 5px;
}
.input-item.error input {
border-color: red;
}
</style>
Copy link
Collaborator

Choose a reason for hiding this comment

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

index.html 리뷰 참고해 주세요!

Comment on lines +86 to +90
id="passwordConfirmation-toggle"
/>
</div>
<div
id="passwordConfirmation-error-message"
Copy link
Collaborator

Choose a reason for hiding this comment

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

camelCase와 kebab-case의 형태가 모두 보이는데 하나로 통일해서 사용해 주세요!

Suggested change
id="passwordConfirmation-toggle"
/>
</div>
<div
id="passwordConfirmation-error-message"
id="password-confirmation-toggle"
/>
</div>
<div
id="password-confirmation-error-message"

Comment on lines +101 to +127
addValidationListeners(
emailInput,
emailErrorMessage,
validateEmail,
"이메일을 입력해주세요.",
"올바른 이메일 형식을 입력해주세요."
);
addValidationListeners(
passwordInput,
passwordErrorMessage,
validatePassword,
"비밀번호를 입력해주세요.",
"비밀번호는 8자 이상이어야 합니다."
);

togglePasswordButton.addEventListener("click", togglePasswordVisibility);

// 초기 버튼 상태 설정
updateButtonState();

// 폼 제출 이벤트 처리
form.addEventListener("submit", (e) => {
e.preventDefault();
if (!loginButton.disabled) {
window.location.href = "/items.html";
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

멘토링 때 설명드린 것처럼, 선언된 자바스크립트 코드가 시작되는 부분이라는 의미에서 이 부분을 한데 묶어서 init과 같은 명칭의 함수로 묶어 관리해주는 것도 하나의 방법일 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

valindation하는 부분이 signin과 많이 겹칠 것 같은데, 이 경우 해당 유틸 함수는 따로 분리해서 export한 후 각 파일에 import해서 적용하는 방식이 더 좋을 것 같습니다.
utils.js와 같이 파일 생성해서 따로 관리하시면 됩니다!

@lang92 lang92 merged commit e3c613e into codeit-bootcamp-frontend:Basic-김태훈 Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants