Fix [#168] 로딩뷰 QA 이슈 수정 #169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -170,7 +170,6 @@ enum StringLiterals { | |||
|
|
|||
| enum Loading { | |||
| static let loadingMessageTitle = "알고 계셨나요?" | |||
| static let loadingMessage1 = "온라인 상에서도 비속어를\n사용하지 않은 사람들이\n실제 행복지수가 더 높다고 해요." | |||
| static let loadingMessage2 = "Don’t be는 그저 온화한 커뮤니티가 아닌\n온화하면서도 재밌을 수 있는 커뮤니티를 지향해요." | |||
| static let loadingMessage3 = "Don’t be의 온화함을 해치는 글이 보인다면\n직접 투명도 기능을 눌러보세요!" | |||
| static let loadingMessage4 = "Don’t be 팀은 온화한 커뮤니티를 만들기 위해\n저희부터 온화한 팀이 되고자 노력하고 있어요." | |||
There was a problem hiding this comment.
이 코드 리뷰에서는 다음과 같은 사항을 고려할 수 있습니다:
- "loadingMessage1" 상수가 삭제되었으며 이에 관련된 사용 또는 의존성이 있는지 확인해야 합니다.
- 문자열의 길이나 내용 변경으로 인한 UI 또는 다른 기능에 영향을 줄 수 있으므로, 설계 또는 문구적인 변화에 대한 유사한 팀 간 검토 요청이 필요할 수 있습니다.
이외의 디테일한 부분은 여러 컨텍스트와 팀 업무 환경에 따라 달라질 수 있습니다.
| @@ -12,8 +12,7 @@ import Lottie | |||
| final class DontBeLoadingView: UIView { | |||
|
|
|||
| private var loadingText: String = "" | |||
| private var loadingTexts = [StringLiterals.Loading.loadingMessage1, | |||
| StringLiterals.Loading.loadingMessage2, | |||
| private var loadingTexts = [StringLiterals.Loading.loadingMessage2, | |||
| StringLiterals.Loading.loadingMessage3, | |||
| StringLiterals.Loading.loadingMessage4, | |||
| StringLiterals.Loading.loadingMessage5, | |||
There was a problem hiding this comment.
이 코드는 "loadingTexts" 배열에서 "loadingMessage1" 아이템을 제거하고 "loadingMessage2" 아이템을 첫 번째 요소로 추가했습니다. 이 변경은 의도된 것인지 확인해야 합니다. 이에 대한 주석이나 변경 로그가 필요할 수 있습니다. 또한, "loadingTexts" 배열 초기화 중에 콤마(,)를 유지하는 일관성이 있어야 합니다.
개선 제안:
- 코드에서 "loadingText" 변수는 현재 사용되지 않는 것 같습니다. 사용하지 않는 변수는 정리하도록 권장합니다.
- 각 메시지가 언제 표시되는지 설명하는 주석을 추가하여 코드의 가독성을 높일 수 있습니다.
| DispatchQueue.main.async { | ||
| loadingView.hide() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰:
func tabBarController(_ tabBarController: UITabBarController, didSelect viewController: UIViewController)메서드 내에서selectedIndex및beforeIndex의 사용은 명확하지 않다.showLoadingView()가 여러 번 호출될 수 있으며 이는 비효율적이다. 이미 loading view가 표시 중이라면 새로운 호출을 무시하는 방법을 고려해야 한다.loadingView.hide { }대신에loadingView.hide()를 사용하여 코드를 더 간단하게 만들 수 있다.hideLoadingView()메서드가 호출되는 시점과 이유를 좀 더 명확히 설명하면 유지보수가 용이해진다.loadingView객체를 필요할 때마다 새로 만드는 대신에 재사용하여 성능을 개선할 수 있다.
개선 제안:
selectedIndex및beforeIndex값의 확실한 정의와 사용 목적을 추가한다.showLoadingView()에서 이미 loading view가 보여지고 있는지 체크하여 중복 호출을 방지한다.loadingView.hide { }를loadingView.hide()로 단순화한다.hideLoadingView()메서드의 호출 시점 및 목적을 주석으로 상세히 기록한다.loadingView객체를 재사용하여 성능을 향상시킨다.
이러한 변경 사항을 적용하면 코드의 가독성과 효율성을 향상시킬 수 있습니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
👻 PULL REQUEST
💻 작업한 내용
📸 스크린샷
📟 관련 이슈