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

refactor: ContentSettingFeature 액션 컨벤션 수정 #141

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

ShapeKim98
Copy link
Contributor

#️⃣연관된 이슈

#129

📝작업 내용

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

  • ContentSettingFeature 액션 컨벤션 적용

스크린샷 (선택)

💬리뷰 요구사항(선택)

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

ex) 메서드 XXX의 이름을 더 잘 짓고 싶은데 혹시 좋은 명칭이 있을까요?

close 이슈번호

@ShapeKim98 ShapeKim98 requested a review from stealmh October 6, 2024 15:03
@ShapeKim98 ShapeKim98 self-assigned this Oct 6, 2024
@ShapeKim98 ShapeKim98 added the Refactor 🏗️ 뚝딱뚝딱 코드 및 구조 수정 label Oct 6, 2024
Comment on lines 190 to 195
.run { send in
for await _ in self.pasteboard.changes() {
let url = try await pasteboard.probableWebURL()
await send(.inner(.linkPopup(url)), animation: .pokitSpring)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

얘도 액션으로 빼서 깔끔하게 가져갈 수 있읍니다

Comment on lines 202 to 208
let isEdit = state.domain.categoryId != nil

if isEdit {
return .send(.async(.컨텐츠_수정_API))
} else {
return .send(.async(.컨텐츠_추가_API))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

isEdit 
? return .action
: return .action

Copy link
Contributor

@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.

리뷰하려고 했는데 보다가 포기했습니다.

concatenate, merge 사용하시는 부분들 전부 가독성 너무 떨어지는 것 같습니다.
순차적으로 혹은 순서와 상관없이 액션을 실행하고 싶은 마음은 충분히 이해는 합니다. 그러면은
주석을 달아주시던지 함수로 빼보시던지 PR로 해당 내용을 소개해주시던지 하나는 해주셔야 된다고 생각합니다.

읽는사람도 생각해주세요

@stealmh stealmh linked an issue Oct 8, 2024 that may be closed by this pull request
14 tasks
@ShapeKim98
Copy link
Contributor Author

리뷰하려고 했는데 보다가 포기했습니다.

concatenate, merge 사용하시는 부분들 전부 가독성 너무 떨어지는 것 같습니다. 순차적으로 혹은 순서와 상관없이 액션을 실행하고 싶은 마음은 충분히 이해는 합니다. 그러면은 주석을 달아주시던지 함수로 빼보시던지 PR로 해당 내용을 소개해주시던지 하나는 해주셔야 된다고 생각합니다.

읽는사람도 생각해주세요

넵..!! 너무 생각없이 적었습니다. 주의하겠습니다..!! 함수로 빼는 방향으로 가겠습니다

@ShapeKim98 ShapeKim98 merged commit 38658ba into develop Oct 15, 2024
1 check passed
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