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

[페이먼츠 미션 Step 3] 윤생(이윤성) 미션 제출합니다. #309

Merged
merged 11 commits into from
May 8, 2023

Conversation

2yunseong
Copy link

@2yunseong 2yunseong commented May 8, 2023

안녕하세요 파랑! 연휴는 잘 보내셨을까요!!

이번 step3 는 2가지를 위주로 진행해보았습니다.

1. 로딩 스피너 구현 & 스토리북 구체화

  • 사실 로딩 스피너 구현 자체로 어디에 적용해보았기 보다는, 로딩 스피너를 하나 만들고 스토리북 문서(mdx)를 통해 개발을 모르는 팀원도 해당 컴포넌트를 쉽게 이해할 수 있도록 할 수 있을지 고민해보았습니다.

2. 모달을 분리 후 npm으로 배포

가장 시간을 많이 쓴 부분인데요 ... 많은 시행착오가 있었습니다. 일단 어찌저찌 성공은 했으나, 아쉬움이 많이 남아요. 다음과 같은 점에서 아쉬움이 많이 남는데요,

  • semantic versioning에 맞지 않게 버전을 관리한 점
  • 무언가 오류가 발생했을 때 체계적인 접근 보다는 무작정 접근해서 해결하려다 오히려 꼬여 시간이 더 들었던 경험
  • 별다른 사전 공부 없이 무모하게 접근한 점

허나 많은 공부가 되었습니다! JS의 import/export 방식들을 다시 보는 기회가 되었고 조금 더 npm 생태계(?)에 가까워진 것 같습니다.

부족한 부분

스토리북 상호작용 테스트까지는 하지 못하였습니다.
npm 배포나 설정 관련해서도 부족한 부분 있으면 코멘트로 남겨주시면 감사드립니다.

링크

  1. npm 배포 링크 : npm - yunseong-bottom-sheet
  2. react-payment 링크 : payments

@InSeong-So InSeong-So self-requested a review May 8, 2023 12:56
@InSeong-So InSeong-So self-assigned this May 8, 2023
Copy link

@InSeong-So InSeong-So left a comment

Choose a reason for hiding this comment

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

크게 코멘트를 할 부분이 없네요!

로딩 스피너 부분에서 많은 고민을 한 흔적이 보여서 신기했는데, PR 코멘트에 잘 적어두어서 이해가 되었습니다. 개발자에게 협업을 요할 때 가장 중요한 기술 중 하나가 문서화인데 의미 있는 시간이 되었으면 좋겠네요🥰

배포한 라이브러리의 unpkg 용량을 보니 아주 흡족합니다. 많은 러닝을 했으리라 생각되네요.

스탭2에서 많이 피드백을 했던 부분이 개선이 되어서 핑퐁할 요소가 보이지 않네요. 전반적으로 리뷰가 적은 이유입니다👍

이대로 머지해도 좋을 것 같아요.
긴 시간 동안 미션 고생 많았어요👏

Comment on lines 15 to +16
navigate('/add');
};
}, [navigate]);

Choose a reason for hiding this comment

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

navigate는 생략해도 괜찮을 것 같네요!

@InSeong-So InSeong-So merged commit 1a40482 into woowacourse:2yunseong May 8, 2023
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