-
Notifications
You must be signed in to change notification settings - Fork 0
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
hotfix: resourcekit 오류 해결 #24
The head ref may contain hidden characters: "hotfix/\uB9AC\uC18C\uC2A4-\uC5D0\uB7EC-\uD574\uACB0"
Conversation
@@ -6,7 +6,7 @@ | |||
// Copyright © 2024 ppac.farmeme. All rights reserved. | |||
// | |||
|
|||
import Foundation | |||
import SwiftUI | |||
|
|||
public struct Font { | |||
public struct Weight { |
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.
이 코드 패치에 대한 간단한 코드 리뷰입니다:
개선 및 제안사항:
-
라이브러리 변경:
Foundation
에서SwiftUI
로 import를 변경하였습니다.- 만약 이 코드는 UI 요소와 관련된 것이고 SwiftUI가 필요해서 수정한 것이라면 적절합니다.
- 기존의 Foundation 라이브러리를 사용하는 다른 코드들이 영향을 받을 수 있으므로, 이 부분을 점검할 필요가 있습니다.
-
모듈 선택:
SwiftUI
는 사용자 인터페이스 관련 작업을 위해 사용됩니다.- 만약 해당 모듈이 프로젝트 구조에 맞는지, 그리고 실제 사용되는지 확인해야 합니다.
-
코드 구조 및 명료성:
public struct Font
내부에public struct Weight
가 있는 형태는 일반적인 폰트 가중치 설정을 위한 것으로 보입니다.- 그러나 실제 사용하는 예시나 추가적인 필드 및 메서드를 함께 검토해야 최종적으로 적절성을 판단할 수 있습니다.
잠재적 버그 위험:
- 의존성 변경:
import Foundation
을 제거하면서, 프로젝트 내 다른 부위에서 의존성이 있을 경우 문제가 발생할 수 있습니다.- 이전에 Foundation의 기능들을 사용하고 있었다면, SwiftUI로 대체되지 않는 부분이 생길 수 있습니다.
전체적으로 코드를 좀 더 통합적으로 살펴보아야 할 필요가 있으며, 특히 기존 Foundation에 의존하는 부분이 있다면 해당 부분을 신경 써야 합니다.
public static let primary = ResourceKitAsset.neutral10.swiftUIColor | ||
public static let secondary = ResourceKitAsset.neutral20.swiftUIColor | ||
public static let primary = ResourceKitAsset.PrimaryColor.neutral10.swiftUIColor | ||
public static let secondary = ResourceKitAsset.PrimaryColor.neutral20.swiftUIColor | ||
} | ||
|
||
} |
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.
코드 리뷰 의견:
-
의존성 관리:
Foundation
대신SwiftUI
를 import한 부분이 있습니다. SwiftUI에서 필요한 것이 아니라면 불필요하게 메모리를 잡아먹을 수 있으니, 해당 변경이 실제로 필요한지 확인해야 합니다.
-
네임스페이스 변경:
- 모든 ResourceKitAsset 속성을
PrimaryColor
네임스페이스로 변경했습니다. 이는 목적에 맞는지는 팀이나 프로젝트 상황에 따라 다를 수 있습니다만, 주요 색상을 한곳으로 모아서 관리하기 때문에 장점이 있을 수 있습니다.
- 모든 ResourceKitAsset 속성을
-
잠재적인 타이포그래피(오타):
public static let dimmer = Color.Text.primary.opacity(40)
에서opacity(40)
가 0 ~ 1 사이의 값이어야 하기 때문에0.4
로 수정이 필요합니다.brandassistive
는 다른 변수 이름과 일관성이 없으므로brandAssistive
로 수정하는 것을 추천드립니다 (카멜 케이스 적용).
-
중복 코드 제거 가능성:
PrimaryColor
네임스페이스로 변경하면서primary
,secondary
,tertiary
등의 동일 색상이 반복적으로 등장하므로, 이를 최소화할 방법을 고려해볼 수 있습니다. 예를 들어 공통 색상 집합을 정의하고, 이를 참조하도록 하면 코드 가독성과 유지보수성이 향상될 수 있습니다.
-
통일성:
PrimaryColor
외에도 다른 컬러 세트를 사용할 계획이 있다면, 이러한 변경들이 다른 곳에서도 일관되게 반영되도록 주의해야 합니다.
코드는 전반적으로 잘 정리되어 있으며, 제안된 몇 가지 개선사항은 가독성과 유지보수성을 높이는 데 도움이 될 것입니다.
What is this PR? 🔍
이슈
설명
Changes 📝
Screenshot 📸
To Reviewers 🙏