[Mission003] 자바스크립트로 Form 과 Modal 에 대한 UI 만들기 by ganeodolu · Pull Request #35 · eastjun-dev/frontend

ganeodolu · 2020-01-29T16:24:12Z

미션3

요구사항

Modal UI

동준님 코드(index.html, style.css 등)를 기반으로 만들었습니다. Modal UI 구현을 위해 사용한 Material Design Dialog를 사용하기 위해서는 dialog인스턴스 생성 후 open()과 autoInit() 메소드가 필요하였습니다.

this.dialog = new mdc.dialog.MDCDialog(document.querySelector('.mdc-dialog')) // dialog 인스턴스생성
this.dialog.open()  // dialog 시작
mdc.autoInit()  // 태그 안에 data-mdc-auto-init="MDCTextField"가 있는 부분의 Material Design 효과사용

관련 링크
https://material.io/develop/web/components/dialogs/
https://material.io/develop/web/components/auto-init/

미션후기

인적사항을 입력하는 부분에 change이벤트를 사용하여 value가 변경되면 다음버튼이 나오도록 하였습니다.
가족구성원 입력 후 +버튼을 눌러야 가족구성원이 추가되는 방식으로 구현하였습니다.
미션하기 완료 전 작성하였던 코드리뷰에서 불필요해보이는 부분은 삭제하였습니다.

이번 미션을 통하여 다양한 form을 사용해보고, Modal UI를 접할 수 있어서 좋았습니다.

modal 튜토리얼(노마드코더)

This reverts commit 0c8c99c.

개인정보 체크 구현 다음버튼 구현

data에 입력값 저장

UserForm, FamilyForm, MyInfoFrom으로 분리

eastjun-dev

호준님의 코드가 발전되가는게 점점 눈에 더 잘 보이는 것 같아요!!!! 👏
이번 코드에서는 우선적으로 2가지만 더 신경써주시면 좋을 것 같아요!

컨벤션
컨벤션이 안지켜지거나, 일관되지 않는 부분들이 있는 것 같아요.
eslint를 활용해주시면 좋을 것 같습니다.
이벤트 리스너에 할당하는 콜백 함수를 분리해주시면 좋을 것 같아요
콜백부분에 익명함수로 넣다보니 해당 부분이 어떤 기능을 하는지 모든 코드를 봐야 알 수 있다는게 좀 아쉬운 것 같습니다. 그리고 한 함수가 15줄 넘어가면서 길어진다면 함수를 분리하는 것도 좋을 것 같아요! 함수가 길어지면 유지보수가 점점 어려워지니깐요 :)

eastjun-dev · 2020-02-09T04:01:46Z

mission003/ganeodolu/form/FamilyForm.js

+import {renderedFamilyTemplate} from '../js/template.js'
+import { TITLENAME } from '../js/constant.js';
+
+export default function FamilyForm({$targetTitle, $targetAddFamilyFormData, $targetAddFamilyButton, $targetFamilyContainer, $targetCompleteButton2, $targetStep2, $targetStep3, data}){


보통 이렇게 파라미터가 많은 경우에는 한줄로 inline하기보다는 아래와 같이 개행해서 한눈에 보기 편하게 하는 경우가 많던데 이렇게 변경해보는건 어떨까요~? :)

export default function FamilyForm( { $targetTitle, $targetAddFamilyFormData, $targetAddFamilyButton, $targetFamilyContainer, $targetCompleteButton2, $targetStep2, $targetStep3, data })

수정 전에는 가로스크롤이 필요했었는데 세로로 변경하는게 한눈에 더 잘보이네요

eastjun-dev · 2020-02-09T04:03:09Z

mission003/ganeodolu/form/FamilyForm.js

+
+    $targetCompleteButton2.addEventListener('click', (e) => {
+        e.preventDefault()
+        if (e.target.classList.contains('mdc-button__ripple')) {


styling을 위한 class 선택자 대신에, 기능이나 역할을 나타내는 class를 추가한다음에 비교해주는게 If문 로직을 한눈에 이해하기 더 좋을 것 같아요 :)

기존 class명 mdc-button__ripple 뒤에 done을 추가하여 DOMTokenList가 done을 포함하고 있을 때 실행이 되도록 변경하였습니다. 코드만 봐도 이해가 잘 될 수 있도록 더 생각하겠습니다. 😋

eastjun-dev · 2020-02-09T04:04:03Z

mission003/ganeodolu/form/MyInfoForm.js

@@ -0,0 +1,24 @@
+import {renderedMyInfoTemplate} from '../js/template.js'


line2처럼 괄고 다음에 공백 컨벤션을 동일하게 맞춰주시면 더 좋을 것 같아요 :)

이번 미션에서는 공백 통일이 잘 안되었네요. 신경쓰도록 하겠습니다. 😔

eastjun-dev · 2020-02-09T04:08:38Z

mission003/ganeodolu/form/UserForm.js

+
+    let agreementCheck = false;
+
+    $targetUserForm.addEventListener('change', (e) => {


$targetUserForm에 change이벤트가 발생하고 나서 일어나는 행위가 익명함수안에 로직이 길게 들어있으니 무슨 일이 벌어지는건지 모든 코드를 다 읽어야지만 알 수 있는게 조금 아쉬운 것 같아요 ㅠ 익명함수내부 구현 부분을 따로 함수로 분리해서 함수의 네이밍을 바인딩하는게 어떨까요?

전화번호와 이메일 입력체크를 확인하는 함수 areFilledEmailAndPhoneForm와 모든 입력항목 및 개인정보체크를 확인하는 areFilledAllForm 함수를 만들어 구조를 단순화 시켰습니다. 훨씬 간단하고 보기가 쉽네요 👍

eastjun-dev · 2020-02-09T04:11:08Z

mission003/ganeodolu/form/UserForm.js

+    $targetCheckBox.addEventListener('click', (e) => {
+        agreementCheck = !agreementCheck
+        let count = 0;
+        for (let inputIndex = 0; inputIndex < $targetInput.length; inputIndex++) {


17~28 line과 로직이 거의 비슷해 보이는데 함수로 분리해서 재사용하는 방식으로 발전시켜보면 어떨까요?

areFilledAllForm 함수를 만들어 중복을 제거하였습니다~

eastjun-dev · 2020-02-09T04:11:46Z

mission003/ganeodolu/form/UserForm.js

+
+    $targetCompleteButton1.addEventListener('click', (e) => {
+        e.preventDefault()
+        for (let inputIndex = 0; inputIndex < 3; inputIndex++) { // 인덱스가 3인 비밀번호는 data에서 제외


주석은 반드시 필요한 경우가 아니면 제거해주시는게 좋을 것 같아요 :)

나중에 보면 기억이 안날 것 같아서 남겨두었는데 주석이 필요없어도 잘 이해되도록 해야겠어요.

eastjun-dev · 2020-02-09T04:13:15Z

mission003/ganeodolu/js/main.js

@@ -0,0 +1,4 @@
+import App from './App.js'
+let data = {Family: { name: [], relation: [] }};


데이터 구조를 보기 쉽게 아래와 같이 정렬해주시면 더 좋을 것 같아요 :)

let data = { Family: { name: [], relation: [] } };

객체형식을 사용할때 풀어서 정렬하도록 신경쓰겠습니다.

eastjun-dev · 2020-02-09T04:14:28Z

mission003/ganeodolu/js/template.js

+}
+
+function renderedMyInfoTemplate(data){
+    let familyRelations = data.Family.name.map((val, idx) => {


아래와 같이 한줄로 줄여쓸수도 있답니다 :)

let familyRelations = data.Family.name.map((val, idx) => `<div>${val} (${data.Family.relation[idx]})</div>`).join('')

대괄호를 쓰는게 아직 편한데 한줄방식도 익숙해지도록 자주 사용해보겠습니다.

eastjun-dev · 2020-02-09T04:15:02Z

mission003/ganeodolu/js/template.js

+    `
+}
+
+export {renderedFamilyTemplate, renderedMyInfoTemplate} 


{}광호 다음에 공백컨벤션을 맞춰주면 더 좋을 것 같아요 :)

코드 마무리하고 자동정렬을 깜빡했네요. 😨

eastjun-dev · 2020-02-09T05:25:19Z

mission003/ganeodolu/index.html

+        </div>
+      </div>
+    </div>
+    <!-- <div class="mdc-dialog__scrim"></div> -->


사용하지 않는 주석은 제거해주셔도 좋을 것 같아요 :)

모달 아닌부분을 클릭시 닫히게 하는 부분인데 테스트시 불편하여 주석처리하였다가 다시 활성화하겠습니다.

공백컨벤션 통일, 객체풀어쓰기

HoseokNa

원래 미션 수행하고 참고해보려고 했는데 아직 수행을 못해서
먼저 코드 확인 후 리뷰어는 아니지만 겸사겸사 리뷰까지 남겨봅니다.
저도 얼른 진행해야 되는데 세션전까지 완료를 목표로 해봐야겠네요.
주로 변수명과 상수에 대해 리뷰를 남겼습니다.
개인적인 의견이니 참고하시길 바랍니다.

HoseokNa · 2020-02-18T08:14:48Z

mission003/ganeodolu/form/FamilyForm.js

+            $targetStep3.classList.remove('hidden')
+        }
+    })
+}


파일에 전체적으로 end of line이 없습니다. 어느새 저는 EOL이 없으면 불편해지는 몸이 되버렸네요 ㅎㅎ 한번 확인 부탁드리겠습니다.

그동안 EOL이 필요한지 몰랐는데 다른분 리뷰에 쓰신 것을 보고나서야 필요성을 알게되었어요. 앞으로 신경쓰도록 하겠습니다. 😄

HoseokNa · 2020-02-18T08:17:06Z

mission003/ganeodolu/form/FamilyForm.js

+        if ($targetFamilyContainer[0].value && $targetFamilyContainer[1].value) {
+            renderedHTMLFamily = renderedFamilyTemplate(renderedHTMLFamily, $targetFamilyContainer)
+            this.data['Family']['name'].push($targetFamilyContainer[0].value)
+            this.data['Family']['relation'].push($targetFamilyContainer[1].value)


$targetFamilyContainer[0], $targetFamilyContainer[1]에서 0이나 1 대신에 상수로 표현되면 개인적으로 코드를 이해하기 더 쉬울 것 같습니다.

$targetFamilyContainer[0]를 familyName으로, $targetFamilyContainer[1]을 familyRelation으로 변경하였습니다. 훨씬 보기가 좋네요!!

HoseokNa · 2020-02-18T08:21:37Z

mission003/ganeodolu/form/FamilyForm.js

+        mdc.autoInit()
+    })
+
+    $targetCompleteButton2.addEventListener('click', (e) => {


Form 별로 변수명이 $targetCompleteButton1, $targetCompleteButton2, $targetCompleteButton3 사용되고 있는데 App에서는 그렇게 사용하고 각 Form 별로는 $targetCompleteButton으로 사용해도 될 것 같은데 다른분들은 어떻게 생각하시는지 궁금하네요

저도 $targetCompleteButton1,2,3이 모두 같은 target이어서 잘 활용하면 좋을텐데 생각만하고 넘어갔었어요. js파일이 단계별로 나눠있으므로 $targetCompleteButton로 통일하여 수정하였습니다. 🤘

HoseokNa · 2020-02-18T08:28:12Z

mission003/ganeodolu/form/UserForm.js

+    $targetUserForm.addEventListener('change', (e) => {
+        let eventName = e.target.name
+        if (eventName === FORMNAME.EMAIL || eventName === FORMNAME.PHONE) {
+            if ($targetPhone.value || $targetEmail.value && $targetAgreement.classList.contains('hidden')) {


해당 로직은 핸드폰 값 혹은 이메일 값이 입력 되어 있고 targetAgreement가 hidden일 경우 조건을 실행하는게 목적으로 보입니다. 개인적으로는 아래와 같이 명시적으로 표현 해주시면 직관적으로 더 이해할 수 있을 것 같습니다.

if( ($targetPhone.value || $targetEmail.value) && $targetAgreement.classList.contains('hidden') )

연산자 순위가 && 연산이 || 보다 높은 걸로 알고 있어서 실제로는 아래와 같이 동작할 것 같습니다. 하지만 연산 결과는 차이가 없는 것 같습니다.

if($targetPhone.value || ($targetEmail.value && $targetAgreement.classList.contains('hidden')) )

명확하게 표현이 안되었는데 괄호가 있으니 보기가 더 좋네요. 연산자 우선순위도 훑어봐야겠어요. 감사합니다. 😃

classList.add('추가') 시 이미 '추가'가 존재하면 무시하고, classList.remove('삭제')시에도 '삭제'가 존재하지 않아도 에러가 나지 않는다고 나오네요. 그래서 조건문 중 세번째 contain 조건은 삭제하였습니다.

HoseokNa · 2020-02-18T08:28:55Z

mission003/ganeodolu/form/UserForm.js

+                count++
+            }
+        }
+        if (count === 4 && agreementCheck) {


4를 상수로 표현하면 무슨 의미인지 더 쉽게 알 수 있을 것 같습니다

원래 옆에 주석을 남겨두었는데, 상수로 표현하면 주석도 필요없고 의미도 전달할 수 있어서 좋을 것 같네요.

HoseokNa · 2020-02-18T08:30:03Z

mission003/ganeodolu/form/UserForm.js

+
+    $targetCompleteButton1.addEventListener('click', (e) => {
+        e.preventDefault()
+        for (let inputIndex = 0; inputIndex < 3; inputIndex++) {


여기서 3도 상수로 표현되면 더 이해하기 쉬울 것 같습니다. 다른 사람의 코드를 보니 상수를 많이 원하게 되네요. 저도 적극 반영해야겠습니다

INPUTCOUNT 상수를 추가하여 INPUTCOUNT.ALL_EXCEPT_PASSWORD로 변경하였습니다. 상수를 적극 활용하도록 하겠습니다~

HoseokNa · 2020-02-18T08:36:45Z

mission003/ganeodolu/js/App.js

+        $targetStep3: $targetStep3, 
+        dialog: this.dialog,
+        data: this.data,
+    })


아래와 같이 줄여 쓰실 수 있습니다.

$targetTitle, $targetMyInfo, $targetCompleteButton3, $targetStep1, $targetStep3, dialog, data,

key와 value가 같을 때 축약형을 수업때 한번 봤었는데 실제 사용하지는 못했었네요. 다른 부분도 많이 줄일 수 있겠어요. 앞으로 잘사용하겠습니다. this.dialog, this.data는 달라서 남겨두었어요.

ganeodolu · 2020-02-18T09:22:04Z

호준님의 코드가 발전되가는게 점점 눈에 더 잘 보이는 것 같아요!!!! 👏
이번 코드에서는 우선적으로 2가지만 더 신경써주시면 좋을 것 같아요!

컨벤션
컨벤션이 안지켜지거나, 일관되지 않는 부분들이 있는 것 같아요.
eslint를 활용해주시면 좋을 것 같습니다.

이벤트 리스너에 할당하는 콜백 함수를 분리해주시면 좋을 것 같아요
콜백부분에 익명함수로 넣다보니 해당 부분이 어떤 기능을 하는지 모든 코드를 봐야 알 수 있다는게 좀 아쉬운 것 같습니다. 그리고 한 함수가 15줄 넘어가면서 길어진다면 함수를 분리하는 것도 좋을 것 같아요! 함수가 길어지면 유지보수가 점점 어려워지니깐요 :)

항상 독려해주셔서 감사합니다. 🙌 그동안 eslint 설치만 해놓고 사용하는 줄 알았는데 설정도 필요했었군요. 코드를 분리하여 잘 이해가 되도록 신경쓰도록 하겠습니다.

ganeodolu · 2020-02-18T09:32:42Z

원래 미션 수행하고 참고해보려고 했는데 아직 수행을 못해서
먼저 코드 확인 후 리뷰어는 아니지만 겸사겸사 리뷰까지 남겨봅니다.
저도 얼른 진행해야 되는데 세션전까지 완료를 목표로 해봐야겠네요.
주로 변수명과 상수에 대해 리뷰를 남겼습니다.
개인적인 의견이니 참고하시길 바랍니다.

EOL, 축약형, 상수표현 등 모르고 지나친 것들을 알려주셔서 많은 도움이 되었습니다. 저도 매의 눈으로 잘 살펴서 개선점을 찾도록 하겠습니다. 😁

class 찾을 때 style보다는 기능을 나타내도록 변경

ganeodolu added 14 commits January 2, 2020 18:50

M3 first commit

0c8c99c

modal 튜토리얼(노마드코더)

Revert "M3 first commit"

2941261

This reverts commit 0c8c99c.

M3 revert commit

6778758

M3 eastjun의 index, css 사용

aa5778d

M3 modal 이해하기

740478e

M3 개인정보폼 구현중

2e1c26e

개인정보 체크 구현 다음버튼 구현

M3 개인정보폼 구현완료

3bf73ca

M3 step2로 넘어가기 구현중

0ba086a

M3 step2 넘어가기 구현중

a6e6c7e

M3 step2 완료

fc5e95d

data에 입력값 저장

M3 구현완료

126bc03

M3 컴포넌트화

48cb66c

UserForm, FamilyForm, MyInfoFrom으로 분리

M3 template, constant 분리

0297f46

M3 form 파일 이동

b7864e6

ganeodolu changed the title ~~Mission003/ganeodolu~~ [Mission003] 자바스크립트로 Form 과 Modal 에 대한 UI 만들기 Jan 29, 2020

ganeodolu self-assigned this Jan 29, 2020

ganeodolu requested review from YongHoonJJo, eastjun-dev and s280493 January 29, 2020 16:25

eastjun-dev requested changes Feb 9, 2020

View reviewed changes

M3 리뷰일부 반영

e071564

공백컨벤션 통일, 객체풀어쓰기

HoseokNa reviewed Feb 18, 2020

View reviewed changes

ganeodolu added 2 commits February 18, 2020 18:04

M3 EOL 추가

67e7076

M3 key, value 같은 부분 축약형 사용

daefe35

M3 리뷰 모두 반영, 함수로 중복제거, 상수추가

6cd3f7f

class 찾을 때 style보다는 기능을 나타내도록 변경

		@@ -0,0 +1,24 @@
		import {renderedMyInfoTemplate} from '../js/template.js'


		let agreementCheck = false;

		$targetUserForm.addEventListener('change', (e) => {

		@@ -0,0 +1,4 @@
		import App from './App.js'
		let data = {Family: { name: [], relation: [] }};

Conversation

ganeodolu commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

미션3

요구사항

Modal UI

미션후기

Uh oh!

eastjun-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganeodolu Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoseokNa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganeodolu commented Jan 29, 2020 •

edited

Loading

ganeodolu Feb 10, 2020 •

edited

Loading

ganeodolu Feb 18, 2020 •

edited

Loading