Skip to content
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

부산대 FE 12조 3주차 PR #6

Merged
merged 1 commit into from
Sep 23, 2024
Merged

부산대 FE 12조 3주차 PR #6

merged 1 commit into from
Sep 23, 2024

Conversation

cla6shade
Copy link
Contributor

안녕하세요 멘토님, 부산대 12조 프론트엔드 이세형입니다.

이번 주차에는 프로젝트 세팅하기, 기본 컴포넌트 만들기 총 두 가지 계획을 세웠었는데요, 다들 추석 본가 이슈로 바빴는지 조금 진행이 더뎌서 기본 컴포넌트는 만들지 못했고, 주말 안으로 개발할 예정입니다.

스텝3에서 처음으로 작성하는 PR이다 보니 설레기도 하고 한편으로는 잘 작성한건지 걱정도 되네요. 앞으로 멘토님께서 제시해주시는 방향대로 잘 나아가보도록 하겠습니다😊

잘 부탁드립니다!

* chore: initial setup

* refactor: 초기 생성된 코드 제거

* chore: eslint 규칙 추가

* chore: eslint 플러그인 추가

* refactor: eslint problem fix

* chore: add vite-tsconfig paths

추가로 잘못된 dependency 수정

* chore: path alias 설정

FSD 구조를 따라 설정

* chore: eslint 무시 규칙 추가

* chore: 폴더 구조 생성
Copy link
Contributor

@taehwanno taehwanno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요! 살펴보고 피드백 드립니다.
저도 잘 부탁드리겠습니다 ㅎㅎ

"scripts": {
"dev": "vite",
"build": "tsc -b && vite build",
"lint": "eslint .",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn lint 실행하니 다수의 error가 검출되고 있네요. 한번 살펴보시고 개선해보면 좋겠습니다.

"@features/*": ["*"],
"@entities/*": ["*"],
"@shared/*": ["*"]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 폴더마다 어떤 기준으로 코드를 관리하게 될지 설명해주시겠어요? 별도 설명은 PR에 없어서 찾아보니 #1 에서 정리했듯 FSD 기반으로 만드신거 같네요. FSD의 이해도에 따라 각자 관점이 다를 수도 있어서 한번 간단하게 나마 정리해서 README에 적어두면 팀에 소속된 팀원 간에 시야도 정렬하는 효과가 있을거라 한번 부탁드려봅니다.

Comment on lines +32 to +33
}
},
Copy link
Contributor

@taehwanno taehwanno Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 FSD는 Isolation of Modules처럼 각 모듈 별로 역할에 따라 다른 모듈을 참조할 수 있거나 없는 의존성 규칙이 있는데요, 이를 사람이 일일이 신경써서 하기보다는 eslint와 같은 도구를 사용해서 자동으로 검출해주는게 좋습니다. 찾아보니 @feature-sliced/eslint-config와 같은 도구가 있긴 한데 한번 검토해봐주시겠어요?

그리고 lint 적용하신다면 git commit 할 때 자동으로 lint 검사 및 autofix 적용될 수 있게 huskylint-staged 적용하는게 좋을 거 같은데 한번 검토해봐주시겠어요?

"dev": "vite",
"build": "tsc -b && vite build",
"lint": "eslint .",
"preview": "vite preview"
Copy link
Contributor

@taehwanno taehwanno Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주말에 추가 진행을 해주신다고 본문에 작성해주셔서 해당 PR이 목표로 하는 범위에 대해 저랑 같이 정렬이 먼저 되는게 좋을 거 같아요. 해당 PR에서 제가 리뷰해드렸으면 하는 부분에 대해 구체적으로 전달해봐주실 수 있을까용? 추후에도 PR 제출해주실 때 PR 본문에 작성된다면 제가 조금 더 말씀해주신 부분을 집중적으로 봐드릴 수 있을 거 같아요!

"version": "0.0.0",
"type": "module",
"scripts": {
"dev": "vite",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여러 인원이 같이 개발하는 걸로 알고 있는데요, 여러 브랜치가 생기면 포맷팅, 린트, 테스트, 빌드가 제대로 동작하는게 중요해서 CI 도구를 적용해보면 어떨까요? Node.js 빌드 및 테스트 문서를 참고하셔서 GitHub Actions를 설정해보면 될 거 같은데 한번 검토 후 의견 부탁드릴게요.

@cla6shade
Copy link
Contributor Author

안녕하세요 멘토님!

꼼꼼하게 남겨주신 코드리뷰 잘 확인했습니다.

주말에 추가 진행을 해주신다고 본문에 작성해주셔서 해당 PR이 목표로 하는 범위에 대해 저랑 같이 정렬이 먼저 되는게 좋을 거 같아요. 해당 PR에서 제가 리뷰해드렸으면 하는 부분에 대해 구체적으로 전달해봐주실 수 있을까용? 추후에도 PR 제출해주실 때 PR 본문에 작성된다면 제가 조금 더 말씀해주신 부분을 집중적으로 봐드릴 수 있을 거 같아요!

원래는 일요일까지 기초 컴포넌트 설계와 페이지 디자인을 어느정도 마친 후 피드백을 받고자 했지만 다들 학교 수업때문에 바쁜지 예정대로 다 완성은 못하게 되었습니다.

따라서 금번 PR에서 멘토님께 리뷰 또는 조언받고자 하는 내용을 아래와 같이 정리하게 되었습니다.

  1. 프로젝트 세팅에서 고려해야 할 내용
  2. 현재 프로젝트 구조에서의 문제점과 개선 방향
  3. shared 컴포넌트를 설계할 때 멘토님은 어떤 과정을 거쳐 설계하시는지

1, 2번의 경우 이미 멘토님께서 조언해주신 부분을 반영하고 있고, 빠른 시일 내로 수정해서 올려보겠습니다.

첫 PR에 피드백받고 싶었던 내용이 많았으나 본의아니게 그 범위를 축소하게 되어 정말 아쉽네요.. 다음 PR에서는 조금 더 열심히 한 후에 피드백 받도록 노력해보겠습니다.

감사합니다!

Copy link
Contributor

@taehwanno taehwanno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로젝트의 다음 단계 진행을 위해 시기상 병합해둘게요 ㅎㅎ 전달드린 부분은 점진적으로 개선하셔서 반영해보시죠. 화이팅입니다.

@taehwanno taehwanno merged commit e15f97c into review Sep 23, 2024
@cla6shade
Copy link
Contributor Author

감사합니다 멘토님! 금주 주말 멘토링때 뵙겠습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants