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

Remind 리펙토링 #130

Merged
merged 14 commits into from
Sep 29, 2024
Merged

Remind 리펙토링 #130

merged 14 commits into from
Sep 29, 2024

Conversation

ShapeKim98
Copy link
Contributor

#️⃣연관된 이슈

#129

📝작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • Remind에 대한 액션들을 한글화 하였습니다.
  • 위임 받은 액션은 DelegateAction이 아닌 ScopeAction으로 이동하였습니다.

스크린샷 (선택)

💬리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

  • 컨벤션에 맞춰 한글화 진행하였는데 피드백 부탁드립니다.
  • ScopeAction에 대해 조금 생각을 해보았습니다. 어느 순간 부터 ScopeAction을 안쓰게 되어 버리기도 했고, DelegateAction은 위임의 역할만 해야 하는데, 제가 아무 생각없이 액션에 대한 코드 수행을 넣고 있어서.. 명확하게 할 필요가 있다 생각했습니다.

close 이슈번호

@ShapeKim98 ShapeKim98 added the Refactor 🏗️ 뚝딱뚝딱 코드 및 구조 수정 label Sep 27, 2024
@ShapeKim98 ShapeKim98 requested a review from stealmh September 27, 2024 09:22
@ShapeKim98 ShapeKim98 self-assigned this Sep 27, 2024
Copy link
Member

@stealmh stealmh left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다!

네이밍 관련해서는 컨텐츠_상세보기_대리자_액션_위임이 명확하긴 하지만 이보다 더 좋은 이름이 있을 것 같아요. 한번 고민해주시면서 수정 해주시면 좋을 것 같습니다. 번역기 돌린것 같달까? 일반적으로 대리자 보다는 delegate가 더 익숙하듯이, delegate는 그대로 들고가도 무방할 것 같다는 의견입니다!

또한 delegateAction에서 처리하는 것이 맞다고 생각합니다. 해당피쳐가 부모에게로 책임을 넘기는 넘기는 액션인데 이 경우에는 자식 API를 호출하는 것이고 부모쪽 피쳐에서 자식 delegate를 실행하지 않기 때문에 delegateAction에서 부모가 수행해야 할 delegate 액션을 작성해도 괜찮을 것 같다는 것이 제 의견입니다.

이번PR은 이름을 변경하는 것이 주요 목표이지만 소소하게 보이는 수정사항들을 약간씩 수정하는 연습해보는 것도 좋을 것 같아요.
예를들어 뷰가 나타났을 때랑 대리자 액션 위임 액션이랑 애니메이션 처리부분 제외하고 겹치는데 이를 함수로 빼보시는 연습 한번 해보면서 수정해주세요.

+++ develop에 첫 merge니까 버전 1.0.4올려서 커밋하나 날려주세엽

@ShapeKim98
Copy link
Contributor Author

고생많으셨습니다!

네이밍 관련해서는 컨텐츠_상세보기_대리자_액션_위임이 명확하긴 하지만 이보다 더 좋은 이름이 있을 것 같아요. 한번 고민해주시면서 수정 해주시면 좋을 것 같습니다. 번역기 돌린것 같달까? 일반적으로 대리자 보다는 delegate가 더 익숙하듯이, delegate는 그대로 들고가도 무방할 것 같다는 의견입니다!

또한 delegateAction에서 처리하는 것이 맞다고 생각합니다. 해당피쳐가 부모에게로 책임을 넘기는 넘기는 액션인데 이 경우에는 자식 API를 호출하는 것이고 부모쪽 피쳐에서 자식 delegate를 실행하지 않기 때문에 delegateAction에서 부모가 수행해야 할 delegate 액션을 작성해도 괜찮을 것 같다는 것이 제 의견입니다.

이번PR은 이름을 변경하는 것이 주요 목표이지만 소소하게 보이는 수정사항들을 약간씩 수정하는 연습해보는 것도 좋을 것 같아요. 예를들어 뷰가 나타났을 때랑 대리자 액션 위임 액션이랑 애니메이션 처리부분 제외하고 겹치는데 이를 함수로 빼보시는 연습 한번 해보면서 수정해주세요.

+++ develop에 첫 merge니까 버전 1.0.4올려서 커밋하나 날려주세엽

  1. 전 정말 네이밍 센스가 없는거 같아요...ㅎㅎ 반영하겠습니다.
  2. 저는 delegate라는 개념 자체가 책임을 전가하는 목적만 있다고 생각을 하고, 내부에선 어떠한 코드도 수행해선 안된다고 생각했었는데... 지금의 상황같이 부모가 자식에 책임을 전가받고, 다른 자식에 다시 전가하는 특수한 상황에서는 말씀주신 내용이 일리가 있는 것 같습니다. 반영하겠습니다. 공부가 많이 되네요.
  3. 넵 한번 반영해보겠습니다!
  4. 맨날 까먹네요... 바로 1.0.4로 바꾸겠습니다!

@ShapeKim98 ShapeKim98 merged commit 1853ce2 into develop Sep 29, 2024
1 check passed
@stealmh stealmh linked an issue Oct 2, 2024 that may be closed by this pull request
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor 🏗️ 뚝딱뚝딱 코드 및 구조 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

액션 컨벤션 정립
2 participants