-
Notifications
You must be signed in to change notification settings - Fork 1
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
[iOS] RewindJourney Scene #110
[iOS] RewindJourney Scene #110
Conversation
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.
고생하셨습니다!
전체적으로 Lint 적용이 안되어 있는 것 같아요.
한 번 적용하고 수정 부탁드립니다 😊
|
||
// MARK: - Constants | ||
|
||
private enum Metrix { |
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.
private enum Metrix { | |
private enum Metric { |
static var verticalInset: CGFloat = 8.0 | ||
static var horizonalInset: CGFloat = 12.0 | ||
static var cornerRadius: CGFloat = 8.0 | ||
|
||
//albumart view | ||
enum AlbumArtView { | ||
static var height: CGFloat = 52.0 | ||
static var width: CGFloat = 52.0 | ||
} | ||
//title view | ||
enum TitleView { | ||
static var height: CGFloat = 4.0 | ||
static var inset: CGFloat = 4.0 | ||
static var titleHight: CGFloat = 24.0 | ||
static var subTitleHight: CGFloat = 20.0 | ||
} | ||
|
||
//playtime view | ||
enum PlayTimeView { | ||
static var width: CGFloat = 67.0 | ||
static var verticalInset: CGFloat = 22.0 | ||
static var horizonalInset: CGFloat = 4.0 | ||
static var conponentsHeight: CGFloat = 24.0 | ||
} |
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.
이 친구들 상수라서 static let
으로 사용해도 될 것 같습니다.
private var albumArtView = UIImageView() | ||
private var titleView = UIStackView() | ||
private var titleLabel = UILabel() | ||
private var subTitleLabel = UILabel() | ||
private var playTimeView = UIStackView() | ||
private var playTimeLabel = UILabel() |
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.
이 친구들도 Component 자체를 갈아끼우는 일은 없을 것 같아서
let
으로 설정해줘도 될 것 같아요!
// MARK: - UI Components: style | ||
|
||
private func configureStyle() { | ||
MSFont.registerFonts() |
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.
registerFonts()
는 SceneDelegate에서 한 번만 해주세요..!
// MARK: - Properties | ||
|
||
private var albumArtView = UIImageView() | ||
private var titleView = UIStackView() |
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.
private var titleView = UIStackView() | |
private let titleStack = UIStackView() |
StackView는 프로퍼티 명에 Stack이 명시적으로 들어가면 좋을 것 같습니다.
|
||
} | ||
|
||
// MARK: - Properties |
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.
// MARK: - Properties | |
// MARK: - UI Components |
var preHighlightenProgressView: MSProgressView? | ||
let leftTouchView = UIButton() | ||
let rightTouchView = UIButton() | ||
|
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.
// MARK: - Properties | |
MARK 주석 용도가 조금 헷갈릴 수 있겠네요..
노션 문서 업데이트 해두겠습니다 😅
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.
MSMusicView랑 MSProgressView는 MSUIKit에 넣어도 될 것 같은데 어떻게 생각하시나요?
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.
현재 저희 앱에선 해당 화면 외에 사용할 일이 없어 굳이 빼지 않는 것이 좋을 것 같아요!
public var percentage: Float = 0.0 { | ||
didSet { | ||
syncProgress(percentage: percentage) | ||
} | ||
} | ||
public var isHighlight: Bool = false { | ||
didSet { | ||
self.percentage = self.isHighlight ? 1.0 : 0.0 | ||
} | ||
} |
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.
이 두 가지 프로퍼티는 내부에서만 사용되는 것 같은데 왜 public
인걸까요?
self.progressViews?[index].isHighlight = true | ||
self.preHighlightenProgressView?.isHighlight = false | ||
self.preHighlightenProgressView = self.progressViews?[index] |
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.
리뷰 노트를 보니 임시 처리인 것 같긴 하지만
인덱스 접근은 그래도 안전장치를 하나씩 마련해두면 좋을 것 같습니다.
progressViews
의 count가 index - 1
보다 크거나 작은 지 확인하는 정도로요..!
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.
앗 안전장치는 아래 코드에 되어있습니다!
@objc private func leftTouchViewTapped() {
guard let presentImageIndex else {
return
}
if presentImageIndex > 0 {
let index = presentImageIndex - 1
self.presentImageIndex = index
}
}
@objc private func rightTouchViewTapped() {
guard let images, let presentImageIndex else {
return
}
if presentImageIndex < images.count - 1 {
let index = presentImageIndex + 1
self.presentImageIndex = index
}
}
…stcampwm2023/iOS01-MusicSpot into iOS/story/RewindJourneyScene fetch from remote
syncProgress(percentage: percentage) | ||
} | ||
} | ||
internal var isHighlight: Bool = false { |
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.
isHighlighted
가 뭔가 문법 상 더 적절할 것 같습니다.
} | ||
|
||
required init?(coder: NSCoder) { | ||
fatalError("MusicSpot은 code-based 로 개발되었습니다.") |
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.
fatalError("MusicSpot은 code-based 로 개발되었습니다.") | |
fatalError("MusicSpot은 code-based로만 작업 중입니다.") |
이것도 통일할까요 😋
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.
해당 내용 swift 컨밴션에 추가해두겠습니다.
private func syncProgress(percentage: Float) { | ||
self.progress = percentage | ||
} |
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.
이 함수는 public
으로 해야하지 않을까요?
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.
percentage에 따라 바 모양을 바꾸어주는 함수입니다.
private으로 내부에서만 사용되어야하는 함수가 맞습니다.
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.
아 그러면 isHighlighted
프로퍼티로 패키지 내부에서만 조정하는건가 보군요?
실제로 동작은 투명한 버튼 두개로 하는거고..!?
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.
아 넵 맞습니다!
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.
넵 알겠습니다
확인했습니다!
} | ||
|
||
private func configureAlbumArtViewStyle() { | ||
self.albumArtView.backgroundColor = .blue |
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.
self.albumArtView.backgroundColor = .blue | |
self.albumArtView.backgroundColor = .msColor(.secondaryBackground) |
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.
이미지를 추가하면 사용자에겐 보이지 않으나
다른 분이 저 albumArtView를 작업했을 때, 해당 View의 범위를 시각적으로 보일 수 있게 임의 색상값을 주었습니다.
생각해보니, 이 부분에서 지우고 Preview 코드에 색상값을 주는 코드를 추가해 가시적으로 하면 좋을 것 같네요!
피드백 감사합니다.
|
||
// MARK: - Constants | ||
|
||
private enum Metric { |
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.
상수들 let
으로 변경해주세요..!
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.
이건.. 죄송합니다
❗ 배경
🔧 작업 내역
MSMusicView
RewindJourneyViewController
MSProgressView
🧪 테스트 방법
사용 예제
이미지를 외부에서 주입받아, 주입받은 이미지들을 차례대로 띄워주도록 하였습니다.
📝 리뷰 노트
MSMusicView
: 대략 프레임만 맞추어두었습니다. 후에 음악이 들어간 후 정밀하게 맞추는 작업이 필요할 수도 있습니다.
RewindJourneyViewController
: 오른쪽과 왼쪽 뷰를 두어, 클릭하여 다른 이미지로 전환할 수 있게 구현하였습니다.
지금은 인덱스로 접근하는 방식을 취하고 있는데, 마음에 안들어서 나중에 꼭 바꾸고 싶네요.
MSProgressView
: 추후 유연성있게 사용하기 위해 커스텀하였습니다.
📸 스크린샷
11/27 이후
: 하이라이트 로직 수정하였습니다.