-
Notifications
You must be signed in to change notification settings - Fork 39
[김진선] sprint4 #129
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 #129
The head ref may contain hidden characters: "Basic-\uAE40\uC9C4\uC120-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.
수고하셨습니다!
의도를 더 명확히 드러내고 코드를 간소화하려는 시도를 더 해보시면 훨씬 좋을것같네요 👍
주요 리뷰 포인트
- 유지보수를 고려한 개발
- debounce 함수 사용 관련 피드백
| const input = e.target.closest("[data-validate]"); | ||
| if (!input) return; | ||
| validateInputs(input); | ||
| debouncedCheckAll(); |
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.
이 상황에서는 Debounce를 사용하는 것이 적절하지 않습니다.
이유는:
- 사용자가 입력한 값에 대한 검증과 피드백은 즉각적인 편이 UX를 생각했을때 좋습니다. 즉, 사용자가 입력한 값에 대한 검증은 올바르지 않은 값인것이 확인되었을때 바로 알려주는것이 좋고, 이 과정에서 불필요한 지연이 발생된다면 오히려 사용자 경험을 저해할 수 있습니다.
- 현재 validation 로직이 구현되어있는것은 정규표현식을 이용한 매우 가벼운 비교 연산입니다. 따라서 성능 부담이 그다지 크지 않기때문에 쓰로틀링/디바운싱을 적용하는 효용을 비교해봤을때, 오히려 사용자 경험을 저해하게되어 좋지 않습니다.
| if (!input) return; | ||
| validateInputs(input); | ||
| debouncedCheckAll(); | ||
| }); |
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.
함수 선언부 블락끼리는 그 사이에 한칸씩 띄워주세요!
| // submit | ||
| submitBtn.addEventListener("click", (e) => { | ||
| e.preventDefault(); | ||
| if (!submitBtn.disabled) { |
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.
굳굳! 예외처리 좋고, redirectMap 객체를 조건문 안에서 선언한것도 괜찮네요 👍
만약 이 객체가 여러 파일에서 재사용될 필요가 있다면 이런식으로 상수화해두고 constants.js와 같은 파일이름으로 모듈화해주는것도 괜찮겠죠?
// constants.js
export const REDIRECT_MAP = {
"/login.html": "/items.html",
"/signup.html": "/login.html",
};| if (passwordInput.type === "password") { | ||
| passwordInput.type = "text"; | ||
| img.src = "/images/btn_visibility_on_24px.png"; | ||
| img.alt = "비밀번호 보이는 중"; | ||
| } else { | ||
| passwordInput.type = "password"; | ||
| img.src = "/images/btn_none_visibility_on_24px.png"; | ||
| img.alt = "비밀번호 숨겨진 상태"; | ||
| } |
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.
if else문을 써서 처리해주는것도 나쁘지않지만, 구조를 생각해봤을때 조건이 여러개 붙는게 아닌 둘중에 하나로 토글되는 방식이니까, 이런 방식을 사용하면 의도가 좀 더 명확히 파악되고 코드도 간결해질수있겠네요 :)
const isPasswordVisible = passwordInput.type === "password";
passwordInput.type = isPasswordVisible ? "text" : "password";
img.src = isPasswordVisible
? "/images/btn_visibility_on_24px.png"
: "/images/btn_none_visibility_on_24px.png";
img.alt = isPasswordVisible ? "비밀번호 보이는 중" : "비밀번호 숨겨진 상태";| const { value, id } = inputEl; | ||
| if (!validateType || !validators[validateType]) 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.
코드 흐름을 좀 더 개선해보려면 38번째라인을 37번째 라인보다 위로 갈 수 있게 순서를 바꾸면 더 좋을것같아요.
| if (!validateType || !validators[validateType]) return; | ||
| const { isValid, message } = validators[validateType](value); | ||
| const errMsg = document.getElementById(`${id}-error`); | ||
| if (errMsg) { |
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.
40, 41사이에 한칸 띄워주세요 :)
| if (isValid) { | ||
| inputEl.classList.remove("input-error"); | ||
| } else { | ||
| inputEl.classList.add("input-error"); | ||
| } |
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.
이 조건문도 classList.toggle을 사용하면 이렇게 간소화할수있답니다!
| if (isValid) { | |
| inputEl.classList.remove("input-error"); | |
| } else { | |
| inputEl.classList.add("input-error"); | |
| } | |
| inputEl.classList.toggle("input-error", !isValid); |
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.
굳굳! 유효성 검사 로직은 프로그램내에서 UI와 결합될 필요없이 재사용될 수 있으니, 따로 분리해두고 모듈화하시는게 좋죠 👍


요구사항
기본
로그인
회원가입
심화
배포
https://starlit-crostata-00fe39.netlify.app/
주요 변경사항
index.html의<picture>태그 대신에<img>태그의srcset과sizes사용하도록 시도해 보았습니다.(제가 원하는 대로 구현이 잘 되지 않아서 질문란에 질문을 적어놓았습니다.🙇🏻♂️mobile, tablet, desktop파일이 나눠지는 구조를 페이지마다css파일을 하나씩만 만들고, 그 안에서 미디어쿼리를 사용해 반응형 레이아웃을 만들어주는 구조로 리팩토링 해봤습니다.background-image속성을img태그로 변경해서 코드를 수정했습니다.'input','blur'일 때마다 함수가 실행되면 비효율적일 것 같아서debounce,이벤트 버블링을 사용해서 최적화를 시도해 보았습니다.스크린샷
로그인
회원가입
멘토에게
index.html의<picture>태그 대신에<img>태그의srcset, sizes만 사용하도록 시도해 보았습니다.제가 의도한 것은 화면 크기에 따라 서로 다른 두 이미지를 완전히 교체하는 것이었는데,
<img>의srcset, sizes를 사용했을 때는DOM안의img src속성이 교체되지 않고 다운로드되는 리소스만 최적화되는 것을 확인했습니다.↓ ↑
저는
srcset, sizes는 같은 이미지의 해상도나 용량을 뷰포트 크기에 맞게 최적화 다운로드하는 기능이고,src자체를 화면 크기에 따라 교체하려면<picture>태그나JavaScript로 직접 처리해야 한다고 이해하고 있습니다.혹시 제가 이해한 내용이 정확한지 검토해주시면 감사하겠습니다. 🙇🏻♂️
index.html에 연결된index.css파일을 재사용성과 유지보수를 높이기 위해 변수 중심으로 리팩토링해 보았습니다.이렇게 리팩토링하는 방식이 괜찮은지 피드백 부탁드립니다!
스프린트 미션4에 해당하는 자바스크립트 함수들을 제 나름대로 모듈화 및 최적화를 해보았습니다. 좀 더 개선할 방법이 있다면 알려주시면 감사하겠습니다🙇🏻
셀프 코드 리뷰를 통해 질문 이어가겠습니다.