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

Fix [#82] 1.1.0버전 최종 QA 반영 #84

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix [#82] 1.1.0버전 최종 QA 반영 #84

wants to merge 5 commits into from

Conversation

binisnil
Copy link
Collaborator

👻 PULL REQUEST

🛠️ PR POINT

  • 강제, 선택 업데이트 로직 추가
  • 엠플리튜드 태깅 완료
  • 밴하기 API Router만 구현해놨습니다
  • 대댓글 작성 후, 바로 댓글 작성하면 대댓글에 작성되는 이슈 해결
  • 페이징 로직 수정
  • 대댓글 삭제하려고 하면 게시글 삭제되는 현상 해결

💡 참고사항

페이징로직 관련해서, 어떻게하면 기존의 구조를 따라가지 않고 깔끔하게 구현할 수 있을 지 고민을 많이 해봤는데, 이미 너무 객체 및 프로퍼티간의 유기성이 너무 강해서 우선 업데이트가 급한대로 온몸비틀기를 해보았습니다...........(이런데 쓰이는 표현이 맞나요...?) 리팩토링할때 처음부터 다시짠다는 생각으로 임해보겠습니다

📸 스크린샷

기능 스크린샷
GIF

📟 관련 이슈

Copy link
Collaborator

@JinUng41 JinUng41 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.

코멘트 확인 부탁드릴게요~
추가로 #81 에 대해서도 코멘트 남겨두었습니다!

Comment on lines +10 to +14
extension Array {
subscript(safe index: Int) -> Element? {
return indices.contains(index) ? self[index] : nil
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오, 이렇게 익스텐션으로 구현하신 이유가 궁금하네요.

Comment on lines +94 to +103
let currentComponents = currentVersion.split(separator: ".").map { Int($0) ?? 0 }
let marketingComponents = marketingVersion.split(separator: ".").map { Int($0) ?? 0 }

let currentMajor = currentComponents[safe: 0] ?? 0
let currentMinor = currentComponents[safe: 1] ?? 0
let currentPatch = currentComponents[safe: 2] ?? 0

let marketingMajor = marketingComponents[safe: 0] ?? 0
let marketingMinor = marketingComponents[safe: 1] ?? 0
let marketingPatch = marketingComponents[safe: 2] ?? 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

nil값을 대체하기 위해 많은 부분에서 기본값으로 0이 선언되어 있는데요.
이렇게 되면, nil일 때의 추적이 어렵고 기본값에 대한 의도치 않는 문제가 발생할 수 있다는 단점이 있을 것 같습니다.
이를 옵셔널 바인딩으로 처리하는 것은 어떨까요?

Comment on lines +56 to +62
for (current, appStore) in zip(currentComponents, appStoreComponents) {
if current < appStore {
return true
} else if current > appStore {
return false
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

zip을 사용하셨군요! 👍

반복문 안에서 두 값이 같을 때의 처리는 없는데, 상관없는 부분일까요?


if replyData.count % 15 == 0 && viewModel.cursor != lastCommentID && (scrollView.contentOffset.y + scrollView.frame.size.height) >= (scrollView.contentSize.height) {
if unFlattenDatas.count % 10 == 0 && viewModel.cursor != lastCommentID && (scrollView.contentOffset.y + scrollView.frame.size.height) >= (scrollView.contentSize.height) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

조건문이 많아서 개행하는 것이 좋아보여요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fix] 1.1.0 버전 최종 QA 사항 반영
2 participants