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

Merge Mara-22 into main #6

Merged
merged 19 commits into from
Jan 24, 2024
Merged

Merge Mara-22 into main #6

merged 19 commits into from
Jan 24, 2024

Conversation

a-honey
Copy link
Member

@a-honey a-honey commented Jan 18, 2024

요구사항

이슈번호: MARA-22

작업내용

  • 모달 atom 추가
  • 토스트 메시지 atom 추가
  • 식자재 모달

@choipureum
Copy link
Member

PR 감사드립니다.
몇가지 건의 드릴게 있어 해당 PR에 코멘트 남깁니다🥰

  1. PR시 PR메시지 및 스쿼시 머지시 메시지를 컨벤션에 맞게 작성하는 것을 건의드립니다. 현재 프로젝트 히스토리가 머지 커밋 히스토리만 남아 히스토리 추적이 어렵습니다. 혜선님과 얘기 후 컨벤션을 정하는 것을 추천드립니다. 현재로서는 커밋 린트 husky 설정이 무의미해집니다.

  2. Jira 티켓 트래킹을 추가할수 있는 PR템플릿을 추가하는 방안을 고려해봐도 좋을 것 같습니다. 리뷰시 PR에서 티켓으로 가는 트래킹이 살짝 번거로운데요.. 해당 부분 추가해주시면 좋을것 같습니다!

  3. 런타임 환경에서 테스트를 못해보면 부득이하게 코드레벨에서만 코드리뷰를 해드리는데요. build ci는 추가되어야 할 것 같습니다. 해당 부분 추가부탁드립니다.

번거로우시겠지만 고려부탁드립니다~!🌸

}

const GreenButton: React.FC<GreenButtonProps> = ({ text, className }) => {
const GreenButton: React.FC<GreenButtonProps> = ({
Copy link
Member

Choose a reason for hiding this comment

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

버튼 컴포넌트는 범용적으로 사용 될 가능성이 높아서,
Button 컴포넌트로 생성하고 props로 컬러를 받아서 사용해도 좋을 것 같아요! ☺️

title: string;
children: React.ReactNode;
}
const IngredientAddItem: React.FC<IngredientAddItemProps> = ({
Copy link
Member

Choose a reason for hiding this comment

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

children을 props로 받아서 감싸는 컴포넌트의 경우에는 가독성을 위해서 container을 붙여서 네이밍을 하시는 것도 추천드려요~!😀

@@ -9,12 +10,13 @@ interface GreenLinkProps {

const GreenLink: React.FC<GreenLinkProps> = ({ text, className }) => {
Copy link
Member

Choose a reason for hiding this comment

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

이 버튼 컴포넌트는 저희 디자인에서 많이 사용 될 것 같은 버튼이라서 link로 한정하기 보다는 공통으로 사용 될 수 있고 알아보기 쉽게 Button 컴포넌트에 onClick props로 넘겨줘도 자주 사용 될 것 같아요 🥺🌈

@a-honey a-honey merged commit 748c168 into main Jan 24, 2024
1 check passed
@a-honey a-honey deleted the MARA-22 branch January 24, 2024 19:10
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.

3 participants