-
Notifications
You must be signed in to change notification settings - Fork 39
[염휘건] Sprint4 #149
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 #149
The head ref may contain hidden characters: "Basic-\uC5FC\uD718\uAC74-sprint4"
Conversation
addiescode-sj
left a comment
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.
수고하셨습니다! 클래스를 사용해보셨군요 👍
유지보수 관점에서 몇가지 피드백과 예시 드려봤어요.
참고해보시고 리팩토링해보시면 좋을것같네요~!
주요 리뷰 포인트
- 유지보수를 고려한 개발
- 모듈화
| // 각 InputFieldHander 생성 | ||
| this.emailHandler = new InputFieldHandler( | ||
| this.form.querySelector('#email'), | ||
| validateEmail, | ||
| 'form-field__input--error', | ||
| 'form-field__error-message' | ||
| ); | ||
|
|
||
| this.passwordHandler = new InputFieldHandler( | ||
| this.form.querySelector('#password'), | ||
| validatePassword, | ||
| 'form-field__input--error', | ||
| 'form-field__error-message' | ||
| ); |
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.
클래스 문법을 사용하셨군요!
이렇게 emailHandler와 passwordHanlder를 직접적으로 생성자 메서드 안에서 써주기보다는 연관된 작업끼리는 초기화를 담당하는 함수 하나로 묶어서 관리해주면 어떨까요?
constructor(formEl) {
this.form = formEl;
this.submitBtn = this.form.querySelector('button[type=submit]');
this._initializeFieldHandlers();
this._attachEvents();
this._updateButtonState();
} _initializeFieldHandlers() {
this.emailHandler = new InputFieldHandler(
this.form.querySelector('#email'),
validateEmail,
'form-field__input--error',
'form-field__error-message'
);
this.passwordHandler = new InputFieldHandler(
this.form.querySelector('#password'),
validatePassword,
'form-field__input--error',
'form-field__error-message'
);
}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.
이렇게 바꿔보면 몇가지 장점이 생깁니다 :)
-
클래스 계층 구조와 역할이 명확해집니다.
FormField는, form 안에서 기본 필드 기능을 제공하는 추상 클래스이고, InputTextField는 FormField를 상속받아 좀 더 구체적으로 구체적인 텍스트 입력 필드에 관련된 기능을 제공하는 클래스입니다. -
코드 재사용 및 기능 확장이 용이해집니다. 폼 전체적으로 공통되는 기능은 FormField에서 관리하고, 해당 공통 기능을 상속받아 구체적인 필드에 특화된 기능을 관리하는것은 FormField를 상속받은 클래스에서만 관리합니다.
-
코드를 수정할때, 클래스 계층간의 역할이 명확하다보니 공통되는 기능을 수정 시 한곳에서만 수정하면되니까 테스트도 용이해지고, 수정 및 확장에 용이해집니다.
| * submit -> 이동 | ||
| */ | ||
| _attachEvents() { | ||
| const inputs = [this.emailHandler.inputEl, this.passwordHandler.inputEl]; |
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.
해당 함수의 경우에 inputs를 함수 내부에서 변수를 만들어 관리하기보다는 필요한 입력 필드들을 매개변수로 받게끔 만들면 외부에서 의존성을 주입하게되니까 훨씬 변경에 유연한 함수를 만들수있을것같아요.
| this.submitBtn = this.form.querySelector('button[type=submit]'); | ||
|
|
||
| // 각 InputFieldHander 생성 | ||
| this.emailHandler = new InputFieldHandler( |
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.
FormValidator, InputFieldHandler의 경우 클래스 계층간의 위계가 명확하지않아 유지보수에 어려움이 생길것같아요.
이왕 클래스 문법을 사용하셨으니 클래스 상속을 활용해 더 체계적이고 객체지향적인 구조로 리팩토링 해볼까요? 즉, 먼저 기본이 되는 최상위 클래스인 FormField 클래스를 만들고, 이를 상속받는 InputTextField 클래스를 구현하는 방식은 어떨까요?
- FormField
export class FormField {
/**
* @param {HTMLInputElement} inputEl - Input element
* @param {string} errorClass - CSS class for error state
* @param {string} errorMsgClass - CSS class for error message
*/
constructor(inputEl, errorClass, errorMsgClass) {
if (!inputEl) {
throw new Error('Input element is required');
}
this.inputEl = inputEl;
this.errorClass = errorClass;
this.errorMsgClass = errorMsgClass;
this.formField = this.inputEl.closest('.form-field');
this.isValid = false;
this._initializeEventListeners();
}
...
}- InputTextField
import { FormField } from './FormField.js';
/**
* InputTextField class
* Handles text input field validation
* Extends FormField with specific validation logic
*/
export class InputTextField extends FormField {
/**
* @param {HTMLInputElement} inputEl - Input element
* @param {function(string): {valid: boolean, message: string}} validator - Validation function
* @param {string} errorClass - CSS class for error state
* @param {string} errorMsgClass - CSS class for error message
*/
constructor(inputEl, validator, errorClass, errorMsgClass) {
super(inputEl, errorClass, errorMsgClass);
this.validator = validator;
}
/**
* Validate the input field using the provided validator
* @returns {boolean} True if input is valid
*/
validate() {
const value = this.getValue();
const { valid, message } = this.validator(value);
if (!valid) {
this._showError(message);
return false;
}
this._clearError();
return true;
}
}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.
해당 클래스는 위에 드린 코멘트 참고해서 상속을 사용한 객체지향 친화적인 패턴으로 리팩토링해보시면 좋을것같아요! :)
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 하는 방식보다는, 이런식으로 객체를 사용해 관련있는 코드끼리 모아놓아서 수정 및 확장에 용이하게 바꿔보고 객체만 모듈화해보는건 어떨까요?
const fieldConfigs = [
{
id: 'email',
validator: validateEmail,
errorClass: 'form-field__input--error',
errorMsgClass: 'form-field__error-message'
},
{
id: 'password',
validator: validatePassword,
errorClass: 'form-field__input--error',
errorMsgClass: 'form-field__error-message'
}
];
질문에 대한 답변
네, common.css는 되도록 전역 스타일만 관리하도록 지켜주시고 컴포넌트화할만한 (재사용에 용이한) 독립적인 UI의 경우 따로 CSS 파일을 만들어 관리해주시는게 좋긴 합니다. 즉 100% 정답은 없지만 제 기준으로는 질문주신 srcset의 경우 이런 방식으로 srcset과 sizes 속성을 브레이크포인트에 맞춰 사용하시면 잘 적용됩니다 :) <img
src="./assets/logo/logo_sm.png"
srcset="./assets/logo/logo_typo.png 767w,
./assets/logo/logo_sm.png 768w"
sizes="(max-width: 767px) 100vw, 768px"
alt="판다마켓 로고"
/> |
배포 링크
요구사항
기본
심화
비밀번호의 문자열이 가려질 때는 눈 모양 아이콘에는 사선이 그어져있고, 비밀번호의 문자열이 보일 때는 사선이 없는 눈 모양 아이콘이 보이도록 합니다.
주요 변경사항
스크린샷
멘토에게
이전 피드백에서 캡슐화 되지 않은 css만을 component화 해서 분리하는건 불필요한 비용을 증가시킨다는 피드백을 받았던걸로 기억합니다.
common.css에서 전역 스타일만 관리한다면, 전역 스타일이 아니거나 완벽히 독립적인 UI 단위가 아닌 경우 클래스가 중복되더라도 page-level에서 관리하는게 나은 선택인가요?
오버헤드 방지를 위해
<picture>태그를 제거하고<img>태그만으로 반응형 이미지 변경을 적용하려고 했지만, 다양한 방법으로 시도해봐도 적용되지 않습니다. 해당 링크 를 보고 트러블슈팅을 진행하려고 했는데 잘 적용되지 않는 것 같습니다.