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

[#84] SettingView 내 ListView 분리 및 추가 리팩터링 #89

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

Conversation

CJiu01
Copy link
Contributor

@CJiu01 CJiu01 commented Feb 10, 2025

📌 Summary

  • Setting ListView 분리
  • 버튼 네이밍 컨벤션 반영
  • 버전 정보 추가
  • 액션 완료시, YDS 토스트 추가
  • 버튼 클릭 영역 전체 cell 부분으로 수정

✍️ Description

💡 PR Point

SettingView 내에 있던 List를 분리하는 과정에서 아래와 같은 이슈가 있습니다.

  1. .toggle 의 경우 Binding<Bool> 타입의 연관값을 받습니다.
enum RightItem {
    case none
    case toggle(isOn: Binding<Bool>)
}
  1. onChange의 of 인자는 Equatable한 값을 전달해야하기 때문에 isOn.wrappedValue로 전달하였습니다.
            switch rightItem {
            ...
            case .toggle(let isOn):
                Toggle("", isOn: isOn)
                  ...
                    // TODO: onChange 액션 추가
                    .onChange(of: isOn.wrappedValue) {
                        action()
                    }
            }
  • 위와 같이 작성하면 아래와 같은 버전 이슈가 발생합니다. .onReceive를 사용하는 방법으로 해결해야할까요?
스크린샷 2025-02-10 오후 5 04 54

📚 Reference

🔥 Test

스크린샷 2025-02-11 오후 3 10 06

@CJiu01 CJiu01 self-assigned this Feb 10, 2025
@CJiu01 CJiu01 added the refactor🪄 리팩토링 label Feb 10, 2025
Comment on lines 83 to 85
.onChange(of: isPushAuthorizationEnabled) {
action()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.onChange(of: isPushAuthorizationEnabled) {
action()
}
.onChange(of: isPushAuthorizationEnabled.wrappedValue) { newValue in
action()
}

이렇게 사용하면 에러가 안나요!

제가 추측하기로는 onChange(of:perform:) 메서드를 사용할 때, of: 매개변수로 전달하는 값이 Equatable 프로토콜을 준수해야 해서 값을 전달할 때 .wrappedValue를 사용하고, 클로저의 매개변수를 newValue로 선언함으로써 Bool 타입임을 명확히 알 수 있어서 에러가 안나는거 같아요...!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.wrappedValue 를 통해 Bool 타입을 명시적으로 받아왔다고 생각했는데 아닌가보군요..!
감사합니다! 저도 조금 더 알아볼께요.

@CJiu01
Copy link
Contributor Author

CJiu01 commented Feb 11, 2025

image

피그마에는 title과 item 사이에 구분선이 존재해서 넣어봤는데 어떤가요?

@LeeJoEun-01
Copy link
Contributor

LeeJoEun-01 commented Feb 11, 2025

image 피그마에는 title과 item 사이에 구분선이 존재해서 넣어봤는데 어떤가요?

피그마 확인해보니까 뭔가 이상하게 들어가 있는거보니! 없애는걸 까먹은거 같아요!
(저의 개인적인 의견이지만!)저는 구분선을 넣는것보다는 피그마처럼 Title 부분의 글씨가 조금 더 두껍거나 어둡게 보이는게 더 나을거 같아요!

@CJiu01
Copy link
Contributor Author

CJiu01 commented Feb 13, 2025

image 피그마에는 title과 item 사이에 구분선이 존재해서 넣어봤는데 어떤가요?

피그마 확인해보니까 뭔가 이상하게 들어가 있는거보니! 없애는걸 까먹은거 같아요! (저의 개인적인 의견이지만!)저는 구분선을 넣는것보다는 피그마처럼 Title 부분의 글씨가 조금 더 두껍거나 어둡게 보이는게 더 나을거 같아요!

그럼 구분선 제거하고, Title 수정하겠습니다!

Comment on lines 86 to 88
// .onReceive(isPushAuthorizationEnabled.projectedValue) { newValue in
// action()
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

주석이 필요한 부분이 아니면 삭제하는게 좋을 것 같아요! 위에 TODO도 마찬가지로요!

}
}

struct ItemModel: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

한 가지 걸리는 건 ItemModel이란 네이밍이 적절할까 고민이긴 한데,, ItemModel하면 뭔가 View가 아닌 느낌이 더 들어서, Row나 RowView와 같은 이름이 더 적절할 것 같아요!

Comment on lines 115 to 125
private extension SettingView.SettingList {
func currentAppVersion() -> String {
if let info: [String: Any] = Bundle.main.infoDictionary,
let currentVersion: String
= info["CFBundleShortVersionString"] as? String {
return currentVersion
}
return "-"
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

해당 부분이 비즈니스 로직이라고 생각하면, SettingReducer에서 처리해서 가져오는게 더 적절하지 않을까요?
여기에 앱 버전을 가져오는 로직이 적절할지 논의해보면 좋을 것 같습니다!

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.

[Refactor] SettingView ListView 분리 및 전체적인 리팩토링
3 participants