-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Refactor/#74/skill search] 프로젝트 작성 Form 기술스택 필드 추가 #83
Conversation
…into refactor/#74/SkillSearch
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.
고생많으셨습니다!!
제가 왈가왈부 하는게 좀 아닌거 같다는 생각이 들 정도로
정교하게 돌아가는 코드인거 같았습니다!
잘 모르는 상태에서 이게 어떻고 저건 어떻고 애매한 리뷰 남기는 것보단
빨리 머지를 하고 실사용을 해보는 것도 좋은 방법일 것 같습니다!
TechstackFields
라는 컴포넌트에서 동적 생성과Input / SearchBox
까지 모두 다루고 있어 컴포넌트가 조금 지저분하긴 한데, 이를 또 컴포넌트화 해서 필요한 메서드나 값을 props로 넘기자니 그게 더 보는 입장에서 헷갈릴 수 있을 것 같은데 의견이 궁금합니다(컴포넌트로 분리했다고 해도 완전히 독립되어서 나중에 재사용 컴포넌트로 사용할 수는 없을 것 같습니다)
일단 이 부분은 제가 잘 모르는 걸 수도 있는데 메인페이지하고 검색페이지가 이 컴포넌트가 아니라 StackSearchBox
를 사용하는거면 이 컴포넌트는 프로젝트 작성 페이지에서만 쓰는거니까 이미 충분히 잘 분리된 것 같습니다!
onClickHandler ~
같은 클릭 이벤트를 처리해야 하는 상황들이 많아서 이름만 보고는 유추가 안되는 것 같아 이벤트에 집중하기보다는 해당 로직에 집중하여 네이밍을 해볼까 하는데, 어떤지 궁금합니다
혹시 handleOnClick
보단 기능을 나타내는 handleOnSearchStack
이런 느낌의 네이밍을 말씀하신건가요? 당연히 찬성입니다! 이벤트 핸들러가 하나면 모를까 여러개가 같이 있어야된다면 기능을 네이밍하는게 올바른 방법 같습니다!
네이밍을 skill에서 techstack으로 변경했는데, 혹시 변경 안된부분이 있다면 말씀해주세요
일단 제가 리뷰하면서 보이진 않았습니다!
기능 구현이 개인적으로 너무 어려워서, 컴포넌트 재사용성이라던지 도메인 분리 등등 그런 부분을 많이 고려 못한 것 같습니다.. 보완점이 있다면 피드백 부탁드리겠습니다
앞서 말씀드렸듯이 이건 제가 이 코드에 대해 아직 이해를 다 못했기 때문에 이렇다할 리뷰를 드리기가 어렵네요... 죄송해요 ㅠㅠㅠ
근데 종혁님이 추상화에 신경을 많이 쓰신다는건 알고 있습니다!
폴더 구조만 보더라도 딱딱 필요한 것들을 추려서 만들었다는 느낌을 많이 받았어요
그래서 저는 종혁님 코드 보면서 잘 배우고 있습니다~ ㅎㅎ
src/pages/ProjectEditPage/components/TechStacksFields/components/StackSearchBox.tsx
Show resolved
Hide resolved
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.
종혁님 구현하시느라 고생하셧어요!
TechstackFields
라는 컴포넌트에서 동적 생성과Input / SearchBox
까지 모두 다루고 있어 컴포넌트가 조금 지저분하긴 한데, 이를 또 컴포넌트화 해서 필요한 메서드나 값을 props로 넘기자니 그게 더 보는 입장에서 헷갈릴 수 있을 것 같은데 의견이 궁금합니다(컴포넌트로 분리했다고 해도 완전히 독립되어서 나중에 재사용 컴포넌트로 사용할 수는 없을 것 같습니다)
저는 지금 방식이 좋은 것 같습니다. 말씀해주신 것처럼 더 헷갈릴것 같아요
onClickHandler ~
같은 클릭 이벤트를 처리해야 하는 상황들이 많아서 이름만 보고는 유추가 안되는 것 같아 이벤트에 집중하기보다는 해당 로직에 집중하여 네이밍을 해볼까 하는데, 어떤지 궁금합니다
해당 로직이 무슨 기능을 하는건지 의도를 알수있도록 핸들러 네이밍을 한다는 것으로 이해했는데 맞나요? 맞다면 저도 찬성입니다!
기능 구현이 개인적으로 너무 어려워서, 컴포넌트 재사용성이라던지 도메인 분리 등등 그런 부분을 많이 고려 못한 것 같습니다.. 보완점이 있다면 피드백 부탁드리겠습니다
리뷰하면서 느낀게 구현이 굉장히 까다로우셨을 것 같습니다. 컴포넌트 분리 관련해서 드릴 말씀은 따로 없고, 앞으로 천천히 리팩토링 해주시면 저도 재차 리뷰하면서 이해해보도록 하겠습니다.
#️⃣연관된 이슈
#47 / 4
close #74
💡 핵심적으로 구현된 사항
➕ 그 외에 추가적으로 구현된 사항
🤔 테스트,검증 && 고민 사항
리뷰 코멘트 이전에 이 사항들 먼저 체크하시고 답변 부탁드립니다!!
아직 스타일 등등 다른 작업들도 진행 중이지만, file change가 너무 길어질 것 같아 일단 로직만 먼저 올렸습니다
기술 스택 필드의 경우 동적으로 생성이 되어야 하는 부분이기 때문에 react-hook-form의
useFieldsArray
를 이용했습니다.techStacks.${index}.data
, fields, append 이런것들은 해당 라이브러리 공식 문서에서 소개하는 방법이라서, 궁금하시면 공식문서 참고해주시면 됩니다techStacks.${index}.data
이런 값들을 통해 현재 fields(배열)에 접근하여 데이터를 가져오고, 업데이트 하는 등의 역할을 해주는 걸로 알고 있습니다.TechStackFields는 Input / SearchBox / 선택된 기술스택 렌더링하는 곳 / 으로 나누어져 있습니다
input은 카테고리(백엔드,프론트...) 입력받는용 / SearchBox는 기술스택 검색하는용
로직을 간단하게 예로 들면, 기술스택 추가 버튼을 클릭 시
fields
라는 배열에{category : '', data : []}
이라는 데이터가 추가되고, 이 fields값에 따라 기술스택 필드 컴포넌트가 동적으로 생성됩니다.input을 입력 시 해당 category 속성이 변경되고,
이후에 기술을 검색해서 클릭 시
append
메서드를 통해 해당 요소의 data라는 속성에{id: 1 , 'name: react'}
이런식으로 추가됩니다submit 시에는 해당 fields에 담겨있는 값이 전달됩니다
질문)
TechstackFields
라는 컴포넌트에서 동적 생성과Input / SearchBox
까지 모두 다루고 있어 컴포넌트가 조금 지저분하긴 한데, 이를 또 컴포넌트화 해서 필요한 메서드나 값을 props로 넘기자니 그게 더 보는 입장에서 헷갈릴 수 있을 것 같은데 의견이 궁금합니다(컴포넌트로 분리했다고 해도 완전히 독립되어서 나중에 재사용 컴포넌트로 사용할 수는 없을 것 같습니다)onClickHandler ~
같은 클릭 이벤트를 처리해야 하는 상황들이 많아서 이름만 보고는 유추가 안되는 것 같아 이벤트에 집중하기보다는 해당 로직에 집중하여 네이밍을 해볼까 하는데, 어떤지 궁금합니다네이밍을 skill에서 techstack으로 변경했는데, 혹시 변경 안된부분이 있다면 말씀해주세요
기능 구현이 개인적으로 너무 어려워서, 컴포넌트 재사용성이라던지 도메인 분리 등등 그런 부분을 많이 고려 못한 것 같습니다.. 보완점이 있다면 피드백 부탁드리겠습니다
📌 PR Comment 작성 시 Prefix for Reviewers