-
Notifications
You must be signed in to change notification settings - Fork 18
refactor: suggest new interface #225
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: suggest new interface #225
Conversation
milooy
left a comment
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.
오 PR로 적어주시니 넘 좋네요 ㅎㅎ
test깨져도 무방하니 간단한 demo코드도 적어주실 수 있나요?
|
@milooy 네엡 작성해보겠습니다~! |
|
추가수정완료) @alledy @hiphapis @milooy 사실 재설계는 5, 6번 작업과 많이 겹쳐서 유의미하게는 진행되지 않았구요 ㅎㅎ.. 2, 3번은 확실히 반영해두었습니다. 사용 예시를 보여드리고 싶어서 storybook을 간단하게 설정해보았습니다. 추가1) 추가2) |
|
file changes에서 .yarn 파일 숨기는 쿼리입니당 |
…ics-provider into enhance/suggest-new-interface
Codecov Report
@@ Coverage Diff @@
## main #225 +/- ##
==========================================
- Coverage 89.55% 88.57% -0.99%
==========================================
Files 20 21 +1
Lines 134 140 +6
Branches 12 13 +1
==========================================
+ Hits 120 124 +4
- Misses 14 16 +2
Continue to review full report at Codecov.
|
src/mixin/analytics.ts
Outdated
| this.client = null; | ||
| } | ||
|
|
||
| static setUp(params: SetUpParams) { |
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.
setUp에서 U이 대문자인 이유가 있나요?
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.
함수 이름이어서 동사가 좋겠다고 생각했습니다! 😃
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.
갠적으론 setup 은 그냥 소문자로 가는게 좋을 것 같아요. setup도 하나의 단어라
U를 대문자로 하는 경우를 못본건 아니지만, 너무 극소수여서..;;
중요한건 아니니, 일단은 머지하고, 담에 다시 얘기 나누는걸로 해요!
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.
네 알겠습니다!
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.
말씀하신대로 네이밍 변경했습니다 ㅎㅎ (setUp -> setup)
|
@hiphapis 리뷰 끝나시면 어프룹 부탁드립니당! |
Description
새로운 모듈 구조 길드에서 논의,제시된 구조들을 짬뽕해서 만들어본 구조입니다!
preset을 통해 새로운 AnalyticsClient를 정의하고,
Analytics.click, Analytics.event 등의 함수를 통해 싱글톤스럽게 호출할 수 있습니다~!
Help Wanted 👀
Related Issues
related with #209
resolve #
fix #
Checklist ✋
{PR type}: {PR title}로 맞췄습니다. (type 예시: feat | fix | BREAKING CHANGE | chore | ci | docs | style | refactor | perf | test) (참고: conventional commits)yarn build,yarn test)