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

[#6] 영화,드라마 선택 화면 구현 #13

Merged
merged 6 commits into from
Jan 26, 2025
Merged

Conversation

NORIKIM
Copy link
Collaborator

@NORIKIM NORIKIM commented Jan 17, 2025

  • 영화와 드라마를 선택하게될 화면(ContentView)을 구현하였습니다.
  • 공통으로 사용하는 다음 화면으로 이동하는 버튼과 필터용 선택 버튼을 모듈화 하였습니다.
  • UIImage와 String extension을 생성하였습니다.

@NORIKIM NORIKIM added this to the 오늘뭐봄 milestone Jan 17, 2025
@NORIKIM NORIKIM requested a review from abdul0986 January 17, 2025 00:49
@NORIKIM NORIKIM self-assigned this Jan 17, 2025
Copy link

@abdul0986 abdul0986 left a comment

Choose a reason for hiding this comment

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

피드백 확인 부탁드립니다. :)


import UIKit

class FilterButton: UIButton {

Choose a reason for hiding this comment

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

FilterButton 클래스를 추후 확장하거나 상속시킬 계획이 없다면 final 키워드를 추가해서 파이널 클래스로 정의해주는 것이 좋습니다.

숙제) 그 이유에 대해서 찾아보시고, 다음 멘토링 때 이야기 나누도록 하시죠 :)

Comment on lines +31 to +32
// let genreView = GenreRouter.createGenre
// contentView?.navigationController?.pushViewController(genreView, animated: true)

Choose a reason for hiding this comment

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

PR 올리실 때에는 임의 주석 처리한 코드는 정리해주시는 것이 좋습니다.


func width() -> CGFloat {
let titleWidth = title.width(size: 20)
let leftRightMargin = 70.0

Choose a reason for hiding this comment

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

Suggested change
let leftRightMargin = 70.0
let leftRightMargin: CGFloat = 70.0

Choose a reason for hiding this comment

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

70.0과 같은 매직 넘버는 상수로 바꾸어 가독성을 높일 수 있습니다. 예를 들어, leftRightMargin 값을 상수로 정의하고 재사용할 수 있습니다.

Suggested change
let leftRightMargin = 70.0
private let leftRightMargin: CGFloat = 70.0

Comment on lines 33 to 39
if isSelected {
self.setTitleColor(.buttonSelectedText, for: .normal)
self.backgroundColor = .buttonSelectedBackground
} else {
self.setTitleColor(.buttonDefaultText, for: .normal)
self.backgroundColor = .buttonDefaultBackground
}

Choose a reason for hiding this comment

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

  1. isSelected에 따라 UI 상태를 변경하는 부분은 좋은 방식입니다. 그러나, 이 함수에서 UI 업데이트 시 중복된 코드가 발생하고 있습니다. setTitleColorbackgroundColor에 대해 같은 조건에 맞는 중복 코드가 두 번 작성되어 있습니다. 이를 함수화하거나 별도의 메서드를 만들어 중복을 줄이는 것이 좋습니다.
  2. 굳이 필요하지 않다면, self를 사용할 필요가 없습니다. self는 메서드나 프로퍼티의 이름과 충돌할 가능성이 있을 때만 사용하는 것이 좋습니다. 이 경우 self 없이도 정상적으로 코드가 작동합니다.

private var tvButton: FilterButton!
private let movie = "영화"
private let tv = "TV"
private lazy var selectedContent = movie

Choose a reason for hiding this comment

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

selectedContent는 선택된 컨텐츠를 저장하는데 사용되지만, movieButtontvButton의 상태 변경에 따라 selectedContent가 변경되는 구조로 판단됩니다. 만약 나중에 선택된 콘텐츠에 따라 다른 처리가 필요하다면, selectedContentenum 타입으로 변경하기를 권장드립니다.

}

// 다음 버튼
let _ = NextButton(location: self)

Choose a reason for hiding this comment

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

이 부분이 잘 이해가지 않네요. 할당하지 않는데 왜 생성되었을까요?

@NORIKIM
Copy link
Collaborator Author

NORIKIM commented Jan 23, 2025

피드백 반영하였습니다!

Copy link

@abdul0986 abdul0986 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~!!!

@NORIKIM NORIKIM merged commit 35d8bce into main Jan 26, 2025
2 checks passed
@NORIKIM NORIKIM linked an issue Jan 26, 2025 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

선택 화면(영화, 드라마 선택) 구현
2 participants