-
Notifications
You must be signed in to change notification settings - Fork 15
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
[넥스트레벨 Outwater] 페이먼츠 미션 STEP1 #10
base: Outwater
Are you sure you want to change the base?
Conversation
@woobottle gh-pages 배포하여 데모링크 추가하였으니, 리뷰하실 때 데모환경에서 동작확인 해보셔도 좋을 것 같습니다. |
저의 경우, page별로 먼저 나누어 생각하고, 컴포넌트 단위를 먼저 생각해본 뒤(Card, CardForm), 컴포넌트에서 필요한 상태는 무엇이 있을까 정도를 생각하고 작성했습니다. (리뷰) validation관련 로직이 퍼져있는데,
=> 1번 2번 같은경우 3번 form.checkValidity() method를 활용할수 있는 방안은 없을지 궁금합니다. |
navigate("/alias"); | ||
}; | ||
|
||
const [modalView, setModalView] = useState(false); |
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.
위 아래 로직과 동떨어진 상태에서 선언된게 아닌가 싶어요.
자칫하면 파악하기 어렵지 않을까 싶어서요
선언 위치가 좀 더 파악하기 쉬운곳이면 어떨까 싶습니다. (ex. 최상단 or 최하단)
import Input from "../components/Input"; | ||
import Modal from "../components/Modal"; | ||
import { findCompany } from "../utils/index.js"; | ||
|
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.
가독성 부분에 대해 언급해주신 부분은 아래의 부분때매 언급해주신거라 판단이 되어요
처음 접했을때는 코드 양이 너무 많아서 두려운 마음이 컸지만
읽다 보니 흐름의 이해가 어렵지 않았습니다.
제가 추측하기에 가독성 언급이 나온것은 Input의 onChange로 인해서 코드양이 방대해진것 같아요.
추상화 레벨을 한단계 높여서 type에 따른 Input의 구분과 onChange를 form에 거는것은 어떨까요?
가독성을 해결할수 있는 방법중 하나가 코드의 추상화 레벨을 높여서 관리하는 것이 아닐까 생각해요.
대신 파일 전체의 추상화 레벨이 같게 유지되어야 해서 어려울수 있는 작업이라 생각합니다.
그렇다면 후자의 form의 onchange로 가는게 나쁘지 않을까 해요
formik의 경우 input들의 handler를 form에 걸어서 활용한 것으로 알고있습니다(혹시 아니라면 말씀해주세요!!)
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.
감사합니다. 우병님 코드에서 추상화수준을 맞춰서 가독성이 훌륭한 것을 보고 많이 감명을 받아서, 배워보고 싶어 이번 리팩토링에 유사하게 적용해보았습니다.
추상화수준이 유사하고, 외부에서 주요 로직을 확인할 수 있다면 가독성이 많이 증가되는 것 같아요.
많이 배웠습니다. 감사합니다 👍
Review
데모 환경 바로가기
VSCode환경 바로가기
안녕하세요, 서인수입니다.
storybook을 제외하고 일반 기능구현까지는 완료하였습니다.
코드 보시면서 조금이라도 읽기 어렵거나, 이해 어려운 부분 있으면 코멘트 남겨주시면 정말 좋을 것 같아요.(가독성 측면)
남이 읽기 편한 코드를 작성하고 싶은데, 제 코드는 스스로 많이 보다보니 적응이 되어서 찾기 어려운 것 같아요 ㅠㅠ
이번 Step을 진행하면서 어려웠던 점 3가지 정도 뽑아봤는데, 의견말씀해주시면 큰 도움 될 것 같아요 ㅎㅎ : )
코드 작성하기 전 어느정도 구조 설계를 하시는 지 궁금합니다.
전체적으로 코드가 정해진 기능구현에 초점이 맞추어져있어 유연하지 못하다는 생각이 제가 작성을 하는 내내 들었습니다. cardData의 상태를 객체형식으로 지정하고, 각 객체의 중첩정도가 다르다보니, 이 cardData를 동적(?)으로 모듈화하여 사용하기 어렵지 않았나 생각합니다..
모든 input 마다 각각의 onChange핸들러가 존재하고, 또 각각의 onChange핸들러는 validation 체크, 상태변경(setState), focus이동등 많은 역할을 담당하고 있다.(우겨 넣은 느낌..)
focus이동하는 코드나 error를 띄우는 코드를 모듈화할 수 있지 않을까? 생각이 드는데, 아직은 어떻게 해야할 지 감이 오지않아 부족함을 느꼈습니다..
(리뷰) validation관련 로직이 퍼져있는데, 1) 길이는 input 태그가 가지고 있는 속성으로, 2) onChange시 dataType에 대한 검증 (number인지) 3) 다음 페이지 이동 시, 빈 값이 있는지 검증, 효율적인 validation 방안이 있을지 궁금합니다..
📝 Requirements
카드 추가
<
(뒤로가기) 버튼 클릭 시, 카드 목록 페이지로 이동한다.-
가 삽입된다.*
로 보여진다.MM / YY
로 placeholder를 적용한다./
가 삽입된다.*
으로 보여진다.*
으로 보여진다.심화 요구사항 (선택사항)
Storybook
단위 테스트